unsafe_recovery: also consider applied index when selects force leader #7920

glorv · 2024-03-14T08:05:51Z

What problem does this PR solve?

Issue Number: close #7919, ref tikv/tikv#16717

What is changed and how does it work?

Change online recovery region stale comparator to select to the peer with maximum last index or applied index to become leader.

Check List

Tests

Unit test

Code changes

Has the configuration change
Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
PR to update pingcap/tiup:
Need to cherry-pick to the release branch

Release note

None.

ti-chi-bot · 2024-03-14T08:05:53Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Connor1996
rleungx

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.

glorv · 2024-03-14T08:06:18Z

/cc @nolouch @Connor1996 PTAL

ti-chi-bot · 2024-03-14T08:06:21Z

@glorv: GitHub didn't allow me to request PR reviews from the following users: PTAL.

Note that only tikv members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @nolouch @Connor1996 PTAL

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.

Signed-off-by: glorv <[email protected]>

rleungx · 2024-04-03T07:08:48Z

Need to update go mod.

pkg/unsaferecovery/unsafe_recovery_controller.go

Connor1996 · 2024-04-03T07:11:05Z

pkg/unsaferecovery/unsafe_recovery_controller.go

+			maxIdxA := typeutil.MaxUint64(a.report.GetRaftState().GetLastIndex(), a.report.AppliedIndex)
+			maxIdxB := typeutil.MaxUint64(b.report.GetRaftState().GetLastIndex(), b.report.AppliedIndex)
+			return int(maxIdxA - maxIdxB)
+		},
 		func(a, b *regionItem) int {
 			return int(a.report.GetRaftState().GetLastIndex()) - int(b.report.GetRaftState().GetLastIndex())


Do we need to compare the last index again?

Yes, if max(applied_index) == max(last_index), we better choose the max(last_index) peer to avoid doing force compact.

Co-authored-by: Connor <[email protected]> Signed-off-by: glorv <[email protected]>

Signed-off-by: glorv <[email protected]>

codecov · 2024-04-03T07:50:44Z

Codecov Report

Merging #7920 (172ba3b) into master (a173a08) will increase coverage by 0.09%.
The diff coverage is 100.00%.

❗ Current head 172ba3b differs from pull request most recent head 512a659. Consider uploading reports for the commit 512a659 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7920      +/-   ##
==========================================
+ Coverage   73.49%   73.59%   +0.09%     
==========================================
  Files         436      436              
  Lines       48525    48529       +4     
==========================================
+ Hits        35665    35715      +50     
+ Misses       9802     9757      -45     
+ Partials     3058     3057       -1

Flag	Coverage Δ
unittests	`73.59% <100.00%> (+0.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

glorv · 2024-04-03T07:55:10Z

@Connor1996 PTAL again, thanks~

Connor1996

LGTM

Connor1996 · 2024-04-07T06:03:03Z

/merge

ti-chi-bot · 2024-04-07T06:03:05Z

@Connor1996: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

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 ti-community-infra/tichi repository.

ti-chi-bot · 2024-04-07T06:03:06Z

This pull request has been accepted and is ready to merge.

Commit hash: 172ba3b

ti-chi-bot · 2024-04-07T06:03:20Z

@glorv: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

ti-chi-bot bot added the release-note-none Denotes a PR that doesn't merit a release note. label Mar 14, 2024

ti-chi-bot bot requested review from HunDunDM and nolouch March 14, 2024 08:06

ti-chi-bot bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 14, 2024

ti-chi-bot bot requested a review from Connor1996 March 14, 2024 08:06

glorv added 2 commits March 14, 2024 16:07

also consider applied index in force leader selection

0dc002e

Signed-off-by: glorv <[email protected]>

Merge branch 'master' of https://github.com/tikv/pd into unsafe-recovery

faec6a1

glorv force-pushed the unsafe-recovery branch from e86e7ab to faec6a1 Compare March 14, 2024 08:07

glorv changed the title ~~Unsafe recovery~~ unsafe_recovery: also consider applied index when selects leader Mar 14, 2024

glorv changed the title ~~unsafe_recovery: also consider applied index when selects leader~~ unsafe_recovery: also consider applied index when selects force leader Mar 14, 2024

glorv mentioned this pull request Mar 28, 2024

Tracking issue for apply committed raft log before persistence tikv/tikv#16717

Open

update kv proto

0acae8a

Signed-off-by: glorv <[email protected]>

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 3, 2024

Merge branch 'master' into unsafe-recovery

6eb1499

rleungx approved these changes Apr 3, 2024

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 3, 2024

Connor1996 reviewed Apr 3, 2024

View reviewed changes

pkg/unsaferecovery/unsafe_recovery_controller.go Outdated Show resolved Hide resolved

Connor1996 reviewed Apr 3, 2024

View reviewed changes

Update pkg/unsaferecovery/unsafe_recovery_controller.go

885aae7

Co-authored-by: Connor <[email protected]> Signed-off-by: glorv <[email protected]>

glorv force-pushed the unsafe-recovery branch from 9098ae1 to 885aae7 Compare April 3, 2024 07:31

go mod tidy

172ba3b

Signed-off-by: glorv <[email protected]>

Connor1996 approved these changes Apr 7, 2024

View reviewed changes

ti-chi-bot bot removed the status/LGT1 Indicates that a PR has LGTM 1. label Apr 7, 2024

ti-chi-bot bot added the status/LGT2 Indicates that a PR has LGTM 2. label Apr 7, 2024

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 7, 2024

Merge branch 'master' into unsafe-recovery

512a659

ti-chi-bot bot merged commit 569f3e0 into tikv:master Apr 7, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unsafe_recovery: also consider applied index when selects force leader #7920

unsafe_recovery: also consider applied index when selects force leader #7920

glorv commented Mar 14, 2024 •

edited

Loading

ti-chi-bot bot commented Mar 14, 2024 •

edited

Loading

glorv commented Mar 14, 2024

ti-chi-bot bot commented Mar 14, 2024

rleungx commented Apr 3, 2024

Connor1996 Apr 3, 2024

glorv Apr 3, 2024

codecov bot commented Apr 3, 2024 •

edited

Loading

glorv commented Apr 3, 2024

Connor1996 left a comment

Connor1996 commented Apr 7, 2024

ti-chi-bot bot commented Apr 7, 2024

ti-chi-bot bot commented Apr 7, 2024

ti-chi-bot bot commented Apr 7, 2024

unsafe_recovery: also consider applied index when selects force leader #7920

unsafe_recovery: also consider applied index when selects force leader #7920

Conversation

glorv commented Mar 14, 2024 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Mar 14, 2024 • edited Loading

glorv commented Mar 14, 2024

ti-chi-bot bot commented Mar 14, 2024

rleungx commented Apr 3, 2024

Connor1996 Apr 3, 2024

Choose a reason for hiding this comment

glorv Apr 3, 2024

Choose a reason for hiding this comment

codecov bot commented Apr 3, 2024 • edited Loading

Codecov Report

glorv commented Apr 3, 2024

Connor1996 left a comment

Choose a reason for hiding this comment

Connor1996 commented Apr 7, 2024

ti-chi-bot bot commented Apr 7, 2024

ti-chi-bot bot commented Apr 7, 2024

ti-chi-bot bot commented Apr 7, 2024

glorv commented Mar 14, 2024 •

edited

Loading

ti-chi-bot bot commented Mar 14, 2024 •

edited

Loading

codecov bot commented Apr 3, 2024 •

edited

Loading