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

region_syncer: fix blocking bugs through ctx #3766

Closed
wants to merge 5 commits into from
Closed

region_syncer: fix blocking bugs through ctx #3766

wants to merge 5 commits into from

Conversation

HunDunDM
Copy link
Member

@HunDunDM HunDunDM commented Jun 11, 2021 •
edited
Loading

What problem does this PR solve?

ref #3697

The region_syncer server sometimes takes 10 minutes to exit. The following possibilities were discovered:

  • When the code is executed in the following order, the ctx corresponding to Mark 3 will not be canceled normally. Therefore, the client cannot immediately disconnect from the server.
    • Mark 1: StartSyncWithLeader
    • Mark 2: StopSyncWithLeader
    • Mark 3: establish success
  • The client has just established a connection with the server, and syncHistoryRegion is in progress. This step currently needs to send all data to end and cannot be aborted.

What is changed and how it works?

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

Code changes

Side effects

Related changes

  • Need to cherry-pick to the release branch

Release note

Fix the bug that region_syncer blocks leader changes.

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 11, 2021
@ti-chi-bot ti-chi-bot requested a review from rleungx June 11, 2021 05:43
nolouch
nolouch reviewed Jun 15, 2021
View reviewed changes
Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

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

Could you list the main changes?

@nolouch
Copy link
Contributor

nolouch commented Jun 21, 2021

Ping

@HunDunDM
Copy link
Member Author

Moved part of PR to #3808 . This PR will focus on fixing bugs caused by ctx.

@HunDunDM
use context instead of quit chan
6b3d505 
Signed-off-by: HunDunDM <hundundm@gmail.com>
@HunDunDM
fix region_syncer client ctx
5ec743d 
Signed-off-by: HunDunDM <hundundm@gmail.com>
@HunDunDM
fix region_syncer server ctx
b96a1f6 
Signed-off-by: HunDunDM <hundundm@gmail.com>
@HunDunDM
revert some code
0a35608 
Signed-off-by: HunDunDM <hundundm@gmail.com>
HunDunDM
HunDunDM commented Jun 22, 2021
View reviewed changes
server/region_syncer/client.go
err = syncStream.Send(&pdpb.SyncRegionRequest{
Header: &pdpb.RequestHeader{ClusterId: s.server.ClusterID()},
Member: s.server.GetMemberInfo(),
StartIndex: s.history.GetNextIndex(),
})
if err != nil {
return syncStream, errs.ErrGRPCSend.Wrap(err).FastGenWithCause()
return nil, errs.ErrGRPCSend.Wrap(err).FastGenWithCause()
}

return syncStream, nil
}

// StartSyncWithLeader starts to sync with leader.
func (s *RegionSyncer) StartSyncWithLeader(addr string) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Mark 1

server/region_syncer/client.go
@@ -40,33 +41,24 @@ const (
// StopSyncWithLeader stop to sync the region with leader.
func (s *RegionSyncer) StopSyncWithLeader() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Mark 2

server/region_syncer/client.go
return nil, errors.WithStack(err)
}

s.mu.Lock()
s.mu.regionSyncerCtx, s.mu.regionSyncerCancel = ctx, cancel
Copy link
Member Author

Choose a reason for hiding this comment

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

Mark 3

@HunDunDM HunDunDM changed the title region_syncer: fix blocking bug and reduce save storage region_syncer: fix blocking bugs through ctx Jun 22, 2021
@HunDunDM
prevent bindStream after stopping
710942b 
Signed-off-by: HunDunDM <hundundm@gmail.com>
@disksing
Copy link
Contributor

disksing commented Jul 24, 2021 •
edited
Loading

It seems it can be more likely block at here

pd/server/region_syncer/client.go

Line 120 in 0a35608

err := s.server.GetStorage().LoadRegionsOnce(s.server.GetBasicCluster().CheckAndPutRegion)
especially for a big cluster that has many regions.

@ti-chi-bot
Copy link
Member

@HunDunDM: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2021
@HunDunDM
Copy link
Member Author

HunDunDM commented Nov 5, 2021

The key part of this PR has been merged in #4175 , and the remaining part will be resubmitted in the new PR.

@HunDunDM HunDunDM closed this Nov 5, 2021
@HunDunDM
Copy link
Member Author

The key code of this PR is merged in #4175 , and the remaining part is merged in #4294 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nolouch nolouch nolouch left review comments

@rleungx rleungx Awaiting requested review from rleungx

Assignees
No one assigned
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@HunDunDM @ti-chi-bot @nolouch @disksing