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

log: group replica selector logging and split not leader errors #929

Merged
merged 2 commits into from
Aug 24, 2023
Merged

log: group replica selector logging and split not leader errors #929

merged 2 commits into from
Aug 24, 2023

Conversation

cfzjywxk
Copy link
Contributor

@cfzjywxk cfzjywxk commented Aug 7, 2023

  1. Refactor and group the logging-related logic into RegionRequestSender and replicaSelector.
  2. Split the totalErros into details for NotLeader error responses,
    from
[total-region-errors="not_leader-nil:3"]

[Prefix: target peer id]-not_leader-[Suffix: Leader information in the returned error and nil if no leader]
[total-region-errors="6-not_leader-nil:1, 4-not_leader-nil:1, 5-not_leader-nil:1"]

@cfzjywxk
Copy link
Contributor Author

cfzjywxk commented Aug 7, 2023

/cc @crazycs520 PTAL

ekexium
ekexium reviewed Aug 11, 2023
View reviewed changes
internal/locate/region_request.go Outdated
replica.store.storeID,
replica.isEpochStale(),
replica.attempts,
replica.epoch,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to use atomic load on this variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reminder. I understand the replica should not be used concurrently as its lifetime is the same as the replicaSelector, but I forget why atomic operations are used for the epoch updating.

@cfzjywxk cfzjywxk force-pushed the error-log-split-notleader branch from 557c9e6 to 826a612 Compare August 11, 2023 09:00
you06
you06 approved these changes Aug 18, 2023
View reviewed changes
Copy link
Contributor

@you06 you06 left a comment

Choose a reason for hiding this comment

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

rest LGTM

internal/locate/region_request3_test.go
@@ -1284,3 +1284,31 @@ func (s *testRegionRequestToThreeStoresSuite) TestSendReqFirstTimeout() {
}
}
}

func (s *testRegionRequestToThreeStoresSuite) TestLogging() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems a manual test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

@cfzjywxk cfzjywxk force-pushed the error-log-split-notleader branch from 826a612 to 9f88a2c Compare August 24, 2023 07:46
@cfzjywxk cfzjywxk merged commit fc88757 into tikv:master Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekexium ekexium ekexium approved these changes

@you06 you06 you06 approved these changes

@zyguan zyguan Awaiting requested review from zyguan

@MyonKeminta MyonKeminta Awaiting requested review from MyonKeminta

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cfzjywxk @you06 @ekexium