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

cdc,retry: fix leader missing by extending region retry duration #5269

Merged
merged 6 commits into from
Apr 30, 2022
Merged

cdc,retry: fix leader missing by extending region retry duration #5269

merged 6 commits into from
Apr 30, 2022

Conversation

overvenus
Copy link
Member

What problem does this PR solve?

Issue Number: close #5230

What is changed and how it works?

  • Fix replication interruption due to leader missing by extending region retry duration

Other changes

  • Remove oldvalue in puller and kv client.
  • Refine retry error info, includes the original error.

Check List

Tests

  • Unit test

Related changes

  • Need to cherry-pick to the release branch

Release note 

Fix replication interruption due to leader missing by extending region retry duration

@overvenus overvenus added type/bugfix This PR fixes a bug. component/kv-client TiKV kv log client component. area/ticdc Issues or PRs related to TiCDC. labels Apr 25, 2022
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Apr 25, 2022 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • 3AceShowHand
  • asddongmen

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 release-note Denotes a PR that will be considered when it comes time to generate release notes. needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. needs-cherry-pick-release-5.1 Should cherry pick this PR to release-5.1 branch. needs-cherry-pick-release-5.2 Should cherry pick this PR to release-5.2 branch. needs-cherry-pick-release-5.3 Should cherry pick this PR to release-5.3 branch. needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. needs-cherry-pick-release-6.0 Should cherry pick this PR to release-6.0 branch. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 25, 2022
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 25, 2022
nongfushanquan
nongfushanquan reviewed Apr 26, 2022
View reviewed changes
cdc/kv/client.go
@@ -294,7 +294,6 @@ type CDCKVClient interface {
ctx context.Context,
span regionspan.ComparableSpan,
ts uint64,
enableOldValue bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove the enableOldValue flag in this PR? Is there an issue related to this item?

Copy link
Member Author

Choose a reason for hiding this comment

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

The enableOldValue param has been deprecated monthes ago, it's a cleanup for dead code.

@overvenus
address comments
ce218b9 
Signed-off-by: Neil Shen <[email protected]>
asddongmen
asddongmen reviewed Apr 26, 2022
View reviewed changes
pkg/retry/options.go Outdated
@@ -66,18 +70,19 @@ func WithBackoffMaxDelay(delayInMs int64) Option {
}

// WithMaxTries configures maximum tries, if tries <= 0 "defaultMaxTries" will be used
Copy link
Contributor

@asddongmen asddongmen Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

if tries <= 0 "defaultMaxTries" will be used seems a wrong description.

asddongmen
asddongmen reviewed Apr 26, 2022
View reviewed changes
pkg/retry/options.go Outdated
defaultBackoffCapInMs = 100.0
defaultMaxTries = 3
defaultBackoffCapInMs = 100.0
defaultMaxTries = math.MaxInt64
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you set this to math.MaxInt64?

@overvenus
address comments
1eee8f8 
Signed-off-by: Neil Shen <[email protected]>
@overvenus
Copy link
Member Author

/run-all-tests

@overvenus
fix old value
8baf733 
Signed-off-by: Neil Shen <[email protected]>
@ti-chi-bot ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 29, 2022
@overvenus
Copy link
Member Author

/run-all-tests

@ti-chi-bot ti-chi-bot mentioned this pull request Apr 30, 2022
Closed
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #5316.

ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Apr 30, 2022
@overvenus @ti-chi-bot
This is an automated cherry-pick of pingcap#5269
6d74e2b 
Signed-off-by: ti-chi-bot <[email protected]>
@ti-chi-bot ti-chi-bot mentioned this pull request Apr 30, 2022
Closed
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #5317.

@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #5318.

ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Apr 30, 2022
@overvenus @ti-chi-bot
This is an automated cherry-pick of pingcap#5269
e53afde 
Signed-off-by: ti-chi-bot <[email protected]>
@ti-chi-bot ti-chi-bot mentioned this pull request Apr 30, 2022
Merged
ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Apr 30, 2022
@overvenus @ti-chi-bot
This is an automated cherry-pick of pingcap#5269
83751ad 
Signed-off-by: ti-chi-bot <[email protected]>
@ti-chi-bot ti-chi-bot mentioned this pull request Apr 30, 2022
Closed
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #5319.

ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Apr 30, 2022
@overvenus @ti-chi-bot
This is an automated cherry-pick of pingcap#5269
a881d1f 
Signed-off-by: ti-chi-bot <[email protected]>
@ti-chi-bot ti-chi-bot mentioned this pull request Apr 30, 2022
Closed
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #5320.

ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Apr 30, 2022
@overvenus @ti-chi-bot
This is an automated cherry-pick of pingcap#5269
573a83b 
Signed-off-by: ti-chi-bot <[email protected]>
@ti-chi-bot ti-chi-bot mentioned this pull request Apr 30, 2022
Closed
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #5321.

ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Apr 30, 2022
@overvenus @ti-chi-bot
This is an automated cherry-pick of pingcap#5269
006a71d 
Signed-off-by: ti-chi-bot <[email protected]>
@ti-chi-bot ti-chi-bot mentioned this pull request Apr 30, 2022
Closed
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #5322.

overvenus added a commit to overvenus/ticdc that referenced this pull request Jun 20, 2022
@overvenus
cdc,retry: fix leader missing by extending region retry duration (pin…
7116e97 
…gcap#5269)

Signed-off-by: Neil Shen <[email protected]>
overvenus added a commit to ti-chi-bot/tiflow that referenced this pull request Jun 20, 2022
@overvenus
cdc,retry: fix leader missing by extending region retry duration (pin…
dbdc975 
…gcap#5269)

Signed-off-by: Neil Shen <[email protected]>
overvenus added a commit to ti-chi-bot/tiflow that referenced this pull request Jun 20, 2022
@overvenus
cdc,retry: fix leader missing by extending region retry duration (pin…
390949e 
…gcap#5269)

Signed-off-by: Neil Shen <[email protected]>
ti-chi-bot added a commit that referenced this pull request Jun 20, 2022
@ti-chi-bot
cdc,retry: fix leader missing by extending region retry duration (#5269…
b373dbb 
…) (#5318)

close #5230
overvenus added a commit to ti-chi-bot/tiflow that referenced this pull request Jun 21, 2022
@overvenus
cdc,retry: fix leader missing by extending region retry duration (pin…
5b51c26 
…gcap#5269)

close pingcap#5230
@overvenus overvenus mentioned this pull request Jun 21, 2022
Merged
overvenus added a commit to overvenus/ticdc that referenced this pull request Jun 21, 2022
@overvenus
cdc,retry: fix leader missing by extending region retry duration (pin…
abaa0b6 
…gcap#5269)

close pingcap#5230
overvenus added a commit to ti-chi-bot/tiflow that referenced this pull request Jun 21, 2022
@overvenus
cdc,retry: fix leader missing by extending region retry duration (pin…
3a725a6 
…gcap#5269)

close pingcap#5230
overvenus added a commit to ti-chi-bot/tiflow that referenced this pull request Jun 23, 2022
@overvenus
cdc,retry: fix leader missing by extending region retry duration (pin…
544e243 
…gcap#5269)

close pingcap#5230
ti-chi-bot pushed a commit that referenced this pull request Jun 23, 2022
@overvenus @ti-chi-bot
cdc,retry: fix leader missing by extending region retry duration (#5269)
c5f4d10 
close #5230
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nongfushanquan nongfushanquan nongfushanquan left review comments

@CharlesCheung96 CharlesCheung96 CharlesCheung96 approved these changes

@asddongmen asddongmen asddongmen approved these changes

@3AceShowHand 3AceShowHand 3AceShowHand approved these changes

@hicqu hicqu Awaiting requested review from hicqu

Assignees
No one assigned
Labels
area/ticdc Issues or PRs related to TiCDC. component/kv-client TiKV kv log client component. needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. needs-cherry-pick-release-5.1 Should cherry pick this PR to release-5.1 branch. needs-cherry-pick-release-5.2 Should cherry pick this PR to release-5.2 branch. needs-cherry-pick-release-5.3 Should cherry pick this PR to release-5.3 branch. needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. needs-cherry-pick-release-6.0 Should cherry pick this PR to release-6.0 branch. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ScanRegions total retry time is too short
7 participants
@overvenus @ti-chi-bot @codecov-commenter @asddongmen @3AceShowHand @CharlesCheung96 @nongfushanquan