-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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.
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
store/copr: batch replica read #42237
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
83df99c
to
eadf87f
Compare
76711e2
to
7f1c08d
Compare
0db539c
to
36d46f2
Compare
@@ -1158,6 +1169,11 @@ func (worker *copIteratorWorker) handleTaskOnce(bo *Backoffer, task *copTask, ch | |||
if len(worker.req.MatchStoreLabels) > 0 { | |||
ops = append(ops, tikv.WithMatchLabels(worker.req.MatchStoreLabels)) | |||
} | |||
if task.redirect2Replica != nil { | |||
req.ReplicaRead = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move the setting busyThreshold=0
here so none of the changes would be missing?
store/copr/coprocessor.go
Outdated
if err != nil { | ||
return err | ||
} | ||
if batchedTask == nil { | ||
return nil | ||
} | ||
if idx, ok := b.store2Idx[batchedTask.storeID]; !ok || len(b.tasks[idx].batchTaskList) >= b.limit { | ||
if b.replicaRead == kv.ReplicaReadFollower { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The replicaRead
is a bit confusing, maybe loadbasedReplicaRetry
like name is more clear.
Signed-off-by: you06 <you1474600@gmail.com>
ccd1bd9
to
67e463f
Compare
worker.storeBatchedFallbackNum.Add(uint64(len(remainTasks))) | ||
if !busyThresholdFallback { | ||
worker.storeBatchedNum.Add(uint64(batchedNum - len(remainTasks))) | ||
worker.storeBatchedFallbackNum.Add(uint64(len(remainTasks))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it supposed to be put in an else clause?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The counter will be increased when handling the responses of the follower tasks.
Signed-off-by: you06 <you1474600@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 4ed6972
|
/retest |
2 similar comments
/retest |
/retest |
What problem does this PR solve?
Issue Number: close #42322
Problem Summary:
Currently, store batched coprocessor is not compatible with load-based replica read.
What is changed and how it works?
This PR makes them compatible.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.