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.

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

Support load-based replica read #675

Merged
merged 8 commits into from
Feb 22, 2023

Conversation

sticnarf
Copy link
Collaborator

@sticnarf sticnarf commented Jan 20, 2023

Refers to tikv/rfcs#105.

Corresponding TiDB PR: pingcap/tidb#40742

@sticnarf sticnarf force-pushed the load-based-replica-read branch 10 times, most recently from fb2f1cf to 6e2bfd8 Compare February 8, 2023 02:19
@sticnarf sticnarf marked this pull request as ready for review February 8, 2023 02:20
@sticnarf sticnarf marked this pull request as draft February 8, 2023 02:32
@sticnarf sticnarf force-pushed the load-based-replica-read branch from 6e2bfd8 to d3c5cfc Compare February 8, 2023 03:19
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf force-pushed the load-based-replica-read branch from d3c5cfc to 38d5bc8 Compare February 8, 2023 03:56
@sticnarf sticnarf marked this pull request as ready for review February 8, 2023 03:56
@@ -233,6 +233,7 @@ func (s *Scanner) getData(bo *retry.Backoffer) error {
ResourceGroupTag: s.snapshot.mu.resourceGroupTag,
RequestSource: s.snapshot.GetRequestSource(),
ResourceGroupName: s.snapshot.mu.resourceGroupName,
BusyThresholdMs: uint32(s.snapshot.mu.busyThreshold.Milliseconds()),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why it is not protected by the RWLock?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I think the code before was actually incorrect. Context should not be set here. It should be set below in L243 instead.

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Comment on lines 267 to 273
// exceeding maxReplicaAttempt
// +-------------------+ || RPC failure && unreachable && no forwarding
// exceeding maxReplicaAttempt
// +-------------------+ || RPC failure && unreachable && no forwarding
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Unexpected formats?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't find how to let go fmt ignore it. I add a bar at the line beginning to work around it.

Copy link
Contributor

@you06 you06 Feb 8, 2023

Choose a reason for hiding this comment

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

You can leave an empty line after the graph and before the following code, so that the formatter won't break the indents.

internal/locate/region_request.go Outdated Show resolved Hide resolved
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
if ctx != nil && ctx.Store != nil {
ctx.Store.markAlreadySlow()
if serverIsBusy := regionErr.GetServerIsBusy(); serverIsBusy != nil {
if s.replicaSelector != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the replicaSelector is nil would the markAlreadySlow be skipped unexpectedly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, this is impossible for TiKV requests. RegionRequestSender always creates a new replicaSelector if not exists.

Copy link
Contributor

@cfzjywxk cfzjywxk left a comment

Choose a reason for hiding this comment

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

LGTM

@you06
Copy link
Contributor

you06 commented Feb 22, 2023

A question about ServerIsBusy.AppliedIndex, is the enhancement of sending it to the follower replicas the further plan in your design?

@sticnarf
Copy link
Collaborator Author

A question about ServerIsBusy.AppliedIndex, is the enhancement of sending it to the follower replicas the further plan in your design?

Yes, it's a future plan.

It's not implemented it in TiKV now because I haven't found a good way to get it efficiently. The gRPC threads don't have access to the engine RaftKv while the threads with RaftKv may be too busy at the time.

Maybe it's possible to move local reader to gRPC threads, but there do have a lot to consider.

@sticnarf sticnarf merged commit a27994e into tikv:master Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants