Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

store: add rateLimiter into txn commit batch actions #11817

Merged
merged 10 commits into from
Aug 28, 2019
Merged

store: add rateLimiter into txn commit batch actions #11817

merged 10 commits into from
Aug 28, 2019

Conversation

cfzjywxk
Copy link
Contributor

@cfzjywxk cfzjywxk commented Aug 21, 2019 •
edited
Loading

What problem does this PR solve?

Sub task of supporting LargeTxn for TiDB.(LargeTxn commiter not implemented yet)
Tiny refactor to add concurrency control for processing batchkeys
Use same rateLimiter implementation as in copIterator like tidb_index_serial_scan_concurrency, tidb_distsql_scan_concurrency usage.
This may not be the final selection, some more detailed info about rate limit is described in issue document, some questions listed below(same with in the issue doc),

    1. a global limiter or one per transaction ?
    1. if there is need to adjust the rate limiter dynamically to make better use of resources considering current load(something like txn scheduler?)
    1. limit the batches "worker count" or limit the batch "new worker per second" ?
    1. refactor little or more to cooperate with new LargeTxn commiter better?

the usage and parameters of batchExecutor will be adjusted with following LargeTxn related changes

What is changed and how it works?

Check List

  • Unit test

Code changes

Side effects

Related changes

@jackysp jackysp changed the title add rateLimiter into txn commit batch actions store: add rateLimiter into txn commit batch actions Aug 22, 2019
@jackysp jackysp added the sig/transaction SIG:Transaction label Aug 22, 2019
coocood
coocood reviewed Aug 22, 2019
View reviewed changes
crazycs520
crazycs520 reviewed Aug 22, 2019
View reviewed changes
crazycs520
crazycs520 reviewed Aug 22, 2019
View reviewed changes
@cfzjywxk cfzjywxk requested review from coocood and crazycs520 August 23, 2019 02:33
coocood
coocood reviewed Aug 23, 2019
View reviewed changes
store/tikv/2pc.go Outdated
@@ -115,6 +115,13 @@ type twoPhaseCommitter struct {
regionTxnSize map[uint64]int
}

// taskProcessor add rateLimiter to control concurrency of txn 2pc
type taskProcessor struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think batchExecutor is a better name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can put more attributes in it, so we can reduce the argument count of process and startWorker.

store/tikv/2pc.go Outdated
rateLimiter *rateLimit
}

type procFunc func(bo *Backoffer, batch batchKeys) error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already singleBatchActionFunc defined in doActionOnBatches. We can move it here.

store/tikv/2pc.go
// process will start worker routine and collect results
func (tp *taskProcessor) process(committer *twoPhaseCommitter, procFn procFunc,
bo *Backoffer, action twoPhaseCommitAction, batches []batchKeys) error {
var err error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can move all the logic in doActionOnBatches here.

@codecov
Copy link

codecov bot commented Aug 23, 2019 •
edited
Loading

Codecov Report

Merging #11817 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #11817   +/-   ##
===========================================
  Coverage   81.5518%   81.5518%           
===========================================
  Files           438        438           
  Lines         95630      95630           
===========================================
  Hits          77988      77988           
  Misses        12166      12166           
  Partials       5476       5476

@cfzjywxk cfzjywxk requested a review from coocood August 23, 2019 06:04
@coocood
Copy link
Member

coocood commented Aug 23, 2019

LGTM

@cfzjywxk
Copy link
Contributor Author

@jackysp @lysu @tiancaiamao PTAL

@lysu lysu removed the status/WIP label Aug 26, 2019
@lysu
Copy link
Contributor

lysu commented Aug 26, 2019

@cfzjywxk maybe take care twoPhaseCommitter retry logic.

for example, 1 prewrite request will be div into 4 group will put into batchExecutor.process and fork 4 worker, but if 1 of 4 request be retry at

tidb/store/tikv/2pc.go

Line 481 in 3ea5ba8

err = c.prewriteKeys(bo, batch.keys)
then it will go to batchExecutor again, then current code will renew a executor, so it seems retry request will be missed in rate limit.

crazycs520
crazycs520 reviewed Aug 26, 2019
View reviewed changes
@cfzjywxk
Copy link
Contributor Author

cfzjywxk commented Aug 26, 2019 •
edited
Loading

@lysu @jackysp
This recursive call make it difficult to tell whether one batch process is really finished
process one batch -> region miss -> two new batches retry -> region miss again -> xxx batches retry again -> ...

I'm considering set some variable in backoffer and make rateLimit backoffed, batchExecutor set rateLimit based on this backoffed value when retry is needed. The implentation on this does not need to change a lot and make executor decoupled with twoPhaseCommitter or others

or just let retry requests processed in the same routine sequentially and do not create workers for it?

PTAL any better ideas, thx

@cfzjywxk
Copy link
Contributor Author

/run-all-tests

@cfzjywxk
Copy link
Contributor Author

/run-unit-test

@cfzjywxk
Copy link
Contributor Author

/run-all-tests

@cfzjywxk cfzjywxk requested review from crazycs520 and lysu August 27, 2019 09:51
crazycs520
crazycs520 reviewed Aug 27, 2019
View reviewed changes
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crazycs520 crazycs520 added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 27, 2019
lysu
lysu approved these changes Aug 28, 2019
View reviewed changes
Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lysu lysu added require-LGT3 Indicates that the PR requires three LGTM. type/enhancement The issue or PR belongs to an enhancement. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Aug 28, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Aug 28, 2019

Your auto merge job has been accepted, waiting for #11886

@sre-bot
Copy link
Contributor

sre-bot commented Aug 28, 2019

/run-all-tests

@sre-bot sre-bot merged commit a62bee1 into pingcap:master Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crazycs520 crazycs520 crazycs520 left review comments

@lysu lysu lysu approved these changes

@tiancaiamao tiancaiamao Awaiting requested review from tiancaiamao

@lonng lonng Awaiting requested review from lonng

@jackysp jackysp Awaiting requested review from jackysp

@coocood coocood Awaiting requested review from coocood

Assignees
No one assigned
Labels
require-LGT3 Indicates that the PR requires three LGTM. sig/transaction SIG:Transaction status/can-merge Indicates a PR has been approved by a committer. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cfzjywxk @coocood @lysu @sre-bot @crazycs520 @jackysp