raftstore: fix reset follow apply_unpersisted_log_limit when demoting from leader by glorv · Pull Request #18241 · tikv/tikv

glorv · 2025-02-21T07:33:58Z

What is changed and how it works?

Issue Number: Close #17868, ref #16717

What's Changed:

NOTE: this PR depends on PR tikv/raft-rs#561

This PR adopt the changes in https://github.com/tikv/raft-rs/pull/561 that directly reset the apply_unpersisted_log_limit limit in raft when leader is demoted. It fix the bug that in the previous impl, the reset is done in "on_role_change" which is called in handle_raft_ready, in the round that leader is demoted, it still return unpersisted comitted entries as at the time, the limit is not reset.
NOTE: once we want to support this feature on follower, we need to remove the original assert as it is incorrect in this case.

Related changes

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

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Release note

None

Signed-off-by: glorv <glorvs@163.com>

glorv · 2025-02-21T07:34:12Z

/hold waiting tikv/raft-rs#561.

glorv · 2025-02-21T07:34:47Z

/cc @Connor1996 @overvenus @LykxSassinator @hbisheng @hhwyt

ti-chi-bot · 2025-02-21T07:34:52Z

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

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

Details

In response to this:

/cc @Connor1996 @overvenus @LykxSassinator @hbisheng @hhwyt

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-sigs/prow repository.

Signed-off-by: glorv <glorvs@163.com>

hhwyt · 2025-02-21T07:53:50Z

components/raftstore/src/store/entry_storage.rs

                    mem_size_change -=
                        (bytes_capacity(&e.data) + bytes_capacity(&e.context)) as i64;
                }
+                // NOTE: if we are going to support apply unpersisted entries on follower,


It's better to move this line to the line before the "assert" line.

hhwyt · 2025-02-21T07:55:11Z

tests/failpoints/cases/test_early_apply.rs

    must_get_equal(&cluster.get_engine(3), b"k150", b"v150");
 }
+
+// Test the consistency of EntryCache when partioned leader constains


Suggested change

// Test the consistency of EntryCache when partioned leader constains

// Test the consistency of EntryCache when partitioned leader constains

hhwyt · 2025-02-21T07:56:40Z

tests/failpoints/cases/test_early_apply.rs

 }
+
+// Test the consistency of EntryCache when partioned leader constains
+// uncommitted propose, after the partion is recovered, it can replicate raft


Suggested change

// uncommitted propose, after the partion is recovered, it can replicate raft

// uncommitted propose, after the partition is recovered, it can replicate raft

hhwyt · 2025-02-21T08:06:57Z

tests/failpoints/cases/test_early_apply.rs

+    let mut msg = dropped_append.lock().unwrap().clone();
+    assert_eq!(msg.get_message().to, 1);
+    // Advance the committed index of the Append msg to trigger the corner case.
+    // I don't find an easy way to trigger this kind of msg, so direct modify the


A practical solution 👍

hhwyt · 2025-02-21T08:24:58Z

typo in the PR descrption: "on_role_change" -> "on_role_changed"

components/raftstore/src/store/peer.rs

Signed-off-by: glorv <glorvs@163.com>

Connor1996

LGTM

LykxSassinator

LGTM

hbisheng

LGTM

hbisheng · 2025-02-24T10:06:29Z

tests/failpoints/cases/test_early_apply.rs

+// Test the consistency of EntryCache when partitioned leader constains
+// uncommitted propose, and after the partition is recovered, it can replicate
+// raft entries for new leader correctly. This case test the corner scenario
+// that partitioned leader receive a new Append msg for new elected leader and
+// the new entries are already committed and overlap with existing entries
+// uncomitted entries in the entry cache, it may cause panic if handles
+// incorrectly. See issue https://github.com/tikv/tikv/issues/17868 for more details.


i'm just a spell-checking bot 🤖

Suggested change

// Test the consistency of EntryCache when partitioned leader constains

// uncommitted propose, and after the partition is recovered, it can replicate

// raft entries for new leader correctly. This case test the corner scenario

// that partitioned leader receive a new Append msg for new elected leader and

// the new entries are already committed and overlap with existing entries

// uncomitted entries in the entry cache, it may cause panic if handles

// incorrectly. See issue https://github.com/tikv/tikv/issues/17868 for more details.

// Test the consistency of EntryCache when partitioned leader contains

// uncommitted propose, and after the partition is recovered, it can replicate

// raft entries for new leader correctly. This case tests the corner scenario

// that partitioned leader receives a new Append msg from new elected leader and

// the new entries are already committed and overlap with existing

// uncommitted entries in the entry cache, it may cause panic if handled

// incorrectly. See issue https://github.com/tikv/tikv/issues/17868 for more details.

hbisheng · 2025-02-24T10:14:42Z

tests/failpoints/cases/test_early_apply.rs

+    block_on_timeout(ch, Duration::from_millis(100)).unwrap_err();
+
+    let fp = "pause_on_peer_collect_message";
+    // pause peer 1 to wait for leader timeout, should new leader


Suggested change

// pause peer 1 to wait for leader timeout, should new leader

// pause peer 1 to wait for leader timeout

overvenus

LGTM, wait for raft-rs change.

overvenus · 2025-02-26T06:19:40Z

components/raftstore/src/store/entry_storage.rs

                    let cached_last = cached.range.end - 1;
+                    // TODO: if we are going to support apply unpersisted entries on
+                    // follower, this assert will not longer be correct.
                    assert!(cached_last < trunc_to_idx);


Suggested change

assert!(cached_last < trunc_to_idx);

assert!(cached_last < trunc_to_idx, "cached_last: {}, trunc_to_idx: {}", cached_last, trunc_to_idx);

Signed-off-by: glorv <glorvs@163.com>

glorv · 2025-02-28T08:11:31Z

/unhold

…ower

glorv · 2025-02-28T08:13:19Z

@hbisheng @LykxSassinator PTAL again as tikv/raft-rs#561 is merged.

ti-chi-bot · 2025-02-28T08:22:32Z

[LGTM Timeline notifier]

Timeline:

2025-02-24 08:38:43.780110821 +0000 UTC m=+258671.733269080: ☑️ agreed by Connor1996.
2025-02-28 08:22:31.554072313 +0000 UTC m=+603299.507230564: ☑️ agreed by hbisheng.

ti-chi-bot · 2025-02-28T08:35:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Connor1996, hbisheng, LykxSassinator

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Connor1996,LykxSassinator]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2025-02-28T08:46:43Z

In response to a cherrypick label: new pull request created to branch release-8.5: #18260.

ti-chi-bot · 2025-02-28T08:46:45Z

In response to a cherrypick label: new pull request created to branch release-8.1: #18261.

… from leader (#18241) (#18260) ref #16717, close #17868 This PR adopt the changes in tikv/raft-rs#561 that directly reset the apply_unpersisted_log_limit limit in raft when leader is demoted. It fix the bug that in the previous impl, the reset is done in "on_role_change" which is called in handle_raft_ready, in the round that leader is demoted, it still return unpersisted comitted entries as at the time, the limit is not reset. NOTE: once we want to support this feature on follower, we need to remove the original assert as it is incorrect in this case. Signed-off-by: glorv <glorvs@163.com> Co-authored-by: glorv <glorvs@163.com>

… from leader (#18241) (#18261) ref #16717, close #17868 This PR adopt the changes in tikv/raft-rs#561 that directly reset the apply_unpersisted_log_limit limit in raft when leader is demoted. It fix the bug that in the previous impl, the reset is done in "on_role_change" which is called in handle_raft_ready, in the round that leader is demoted, it still return unpersisted comitted entries as at the time, the limit is not reset. NOTE: once we want to support this feature on follower, we need to remove the original assert as it is incorrect in this case. Signed-off-by: glorv <glorvs@163.com> Co-authored-by: glorv <glorvs@163.com>

fix reset follow apply_unpersisted_log_limit when demoting from leader

4e34901

Signed-off-by: glorv <glorvs@163.com>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the dco. labels Feb 21, 2025

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 21, 2025

ti-chi-bot bot requested review from Connor1996, LykxSassinator, hbisheng and overvenus February 21, 2025 07:34

glorv added 2 commits February 21, 2025 15:42

update deny

8d604a9

Signed-off-by: glorv <glorvs@163.com>

fix format

65360f2

Signed-off-by: glorv <glorvs@163.com>

hhwyt reviewed Feb 21, 2025

View reviewed changes

LykxSassinator reviewed Feb 21, 2025

View reviewed changes

components/raftstore/src/store/peer.rs Show resolved Hide resolved

glorv added 2 commits February 24, 2025 10:51

add comments

ac07161

Signed-off-by: glorv <glorvs@163.com>

fix

6da2abe

Signed-off-by: glorv <glorvs@163.com>

Connor1996 approved these changes Feb 24, 2025

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Feb 24, 2025

LykxSassinator reviewed Feb 24, 2025

View reviewed changes

hbisheng mentioned this pull request Feb 24, 2025

reset max_apply_unpersisted_log_limit in become_follower tikv/raft-rs#561

Merged

hbisheng reviewed Feb 24, 2025

View reviewed changes

overvenus reviewed Feb 27, 2025

View reviewed changes

update raft-rs

ea58a02

Signed-off-by: glorv <glorvs@163.com>

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2025

Merge branch 'master' of https://github.com/tikv/tikv into reset-foll…

d494448

…ower

hbisheng approved these changes Feb 28, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Feb 28, 2025

LykxSassinator approved these changes Feb 28, 2025

View reviewed changes

ti-chi-bot bot merged commit 14622f3 into tikv:master Feb 28, 2025
8 checks passed

ti-chi-bot bot added this to the Pool milestone Feb 28, 2025

glorv added needs-cherry-pick-release-8.1 Should cherry pick this PR to release-8.1 branch. needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. labels Feb 28, 2025

glorv deleted the reset-follower branch February 28, 2025 08:46

ti-chi-bot mentioned this pull request Feb 28, 2025

raftstore: fix reset follow apply_unpersisted_log_limit when demoting from leader (#18241) #18260

Merged

9 tasks

ti-chi-bot mentioned this pull request Feb 28, 2025

raftstore: fix reset follow apply_unpersisted_log_limit when demoting from leader (#18241) #18261

Merged

9 tasks

	// Test the consistency of EntryCache when partioned leader constains
	// Test the consistency of EntryCache when partitioned leader constains

	// uncommitted propose, after the partion is recovered, it can replicate raft
	// uncommitted propose, after the partition is recovered, it can replicate raft

	// pause peer 1 to wait for leader timeout, should new leader
	// pause peer 1 to wait for leader timeout

	assert!(cached_last < trunc_to_idx);
	assert!(cached_last < trunc_to_idx, "cached_last: {}, trunc_to_idx: {}", cached_last, trunc_to_idx);

Conversation

glorv commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is changed and how it works?

Related changes

Check List

Release note

Uh oh!

glorv commented Feb 21, 2025

Uh oh!

glorv commented Feb 21, 2025

Uh oh!

ti-chi-bot bot commented Feb 21, 2025

Uh oh!

hhwyt Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

hhwyt Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

hhwyt Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

hhwyt Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

hhwyt commented Feb 21, 2025

Uh oh!

Uh oh!

Connor1996 left a comment

Choose a reason for hiding this comment

Uh oh!

LykxSassinator left a comment

Choose a reason for hiding this comment

Uh oh!

hbisheng left a comment

Choose a reason for hiding this comment

Uh oh!

hbisheng Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

hbisheng Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

overvenus left a comment

Choose a reason for hiding this comment

Uh oh!

overvenus Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

glorv commented Feb 28, 2025

Uh oh!

glorv commented Feb 28, 2025

Uh oh!

ti-chi-bot bot commented Feb 28, 2025

[LGTM Timeline notifier]

Uh oh!

ti-chi-bot bot commented Feb 28, 2025

Uh oh!

Uh oh!

ti-chi-bot commented Feb 28, 2025

Uh oh!

ti-chi-bot commented Feb 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

glorv commented Feb 21, 2025 •

edited

Loading