raftstore-v2: support tracing peer lifetime #14056

BusyJay · 2023-01-16T07:29:05Z

What is changed and how it works?

Issue Number: Ref #12842

What's Changed:

In V1, a peer is responsible to destroy itself. The design is to make leader do
less work and reduce writes. But from the practice of the pass years, not
making it a strong guarantee actually makes the implementation
complicated and hard to be correct and difficult to understand.

In V2, we changes to make leader the very role to make sure all removed
peers or merged peers must be destroyed in the end. Push mode is way
easier to understand and implement correctly. The downside is extra writes
are introduced but it's worthy.

The PR contains implementation of merge cleanup. But the initialize part should be done in #13818 and its related PRs.

Check List

Tests

Unit test
Integration test

Release note

None

In V1, a peer is responsible to destroy itself. The design is to make leader do less work and reduce writes. But from the practice of the pass years, not making it a strong guarantee actually makes the implementation complicated and hard to be correct and difficult to understand. In V2, we changes to make leader the very role to make sures all removed peers or merged peers must be destroyed in the end. Push mode is way easier to understand and implement correctly. The downside is extra writes are introduced but it's worthy. Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

ti-chi-bot · 2023-01-16T07:29:06Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

tabokie
tonyxuqqi

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.

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

BusyJay · 2023-01-29T06:05:59Z

PTAL

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

overvenus · 2023-01-29T09:33:31Z

In V2, we changes to make leader the very role to make sure all removed
peers or merged peers must be destroyed in the end.

What if restarting tikv cluster after majority peers were destroyed? How does tikv GC remaining peers?

BusyJay · 2023-01-29T09:56:15Z

What if restarting tikv cluster after majority peers were destroyed?

How can that happen? Peers are destroyed by conf change. As long as conf change can proceed, there must be majorities.

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

tabokie · 2023-01-30T08:33:56Z

components/raftstore-v2/src/batch/store.rs

@@ -75,6 +75,7 @@ pub struct StoreContext<EK: KvEngine, ER: RaftEngine, T> {
    pub schedulers: Schedulers<EK, ER>,
    /// store meta
    pub store_meta: Arc<Mutex<StoreMeta<EK>>>,
+    pub shutdown: Arc<AtomicBool>,


Where is this set? And FYI there's already a shutdown in StoreSystem.

It's the exact same bool flag. It's for query only.

tabokie · 2023-01-30T09:06:11Z

components/raftstore-v2/src/operation/ready/mod.rs

@@ -527,10 +557,27 @@ impl<EK: KvEngine, ER: RaftEngine> Peer<EK, ER> {
                .flat_map(|m| self.build_raft_message(m))
                .collect();
        }
+        if self.has_pending_messages() {
+            if write_task.messages.is_empty() {


Avoid unnecessary allocation and memcopy.

tabokie · 2023-01-30T09:06:56Z

components/raftstore-v2/src/operation/ready/mod.rs

-            // return;
+            match msg.get_extra_msg().get_type() {
+                ExtraMessageType::MsgGcPeerResponse => self.on_gc_peer_response(&msg),
+                _ => unimplemented!(),


What about MsgGcPeerRequest?

tabokie · 2023-01-30T09:15:46Z

components/raftstore-v2/src/operation/life.rs

+    Some(msg)
+}
+
+fn tell_source_peer_to_destroy<EK, ER, T>(ctx: &mut StoreContext<EK, ER, T>, msg: &RaftMessage)


merge_source_peer, source/target can be confused with msg sender/recipient.

source/target is referring merge within this project unless stated explicitly.

You used target to represent message receiver at L309.

+1. There's no hurt to make it more explicit as this file is not about merge. Or, at least have a comment to clarify source_peer is merge source peer.

Also regarding the naming, tell is a bit too oral and maybe request is better.

I have already update the target usage at L309. source peer and target peer are very frequent within in the project. It's more ambiguous to make it represent different meanings.

tell is actually a technical term in actor. But I will change it to forward instead.

Even in existing code, it is rare to find usage of "source" directly without hinting the context of merge. Here this function alone is confusing to grasp because neither the implementation nor the function name implies such context. You have to trace it back to caller to get the context, which is not ideal. I think here the least you could do is adding some comments.

it is rare to find usage of "source" directly without hinting the context of merge.

I think this explains "source/target is referring merge within this project unless stated explicitly" perfectly. A comment is also OK.

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

tonyxuqqi · 2023-01-31T00:12:23Z

components/raftstore-v2/src/operation/command/admin/conf_change.rs

+                .iter()
+                .all(|p1| p1.get_store_id() != p0.get_store_id())
+            {
+                removed_records.push(p0.clone());


Does it need dedup?

No, it's impossible to be duplicated.

tonyxuqqi · 2023-01-31T00:23:55Z

components/raftstore-v2/src/operation/life.rs

@@ -260,7 +306,223 @@ impl Store {
    }
 }

+/// Tell leader that `to_peer` is destroyed.
+fn report_peer_destroyed(tombstone_msg: &mut RaftMessage) -> Option<RaftMessage> {


minor: I think the fn name could be renamed as create_raft_tombstone_message(), as there's no report logic in this function at all.

tonyxuqqi · 2023-01-31T01:24:11Z

components/raftstore-v2/src/operation/command/admin/mod.rs

@@ -110,6 +111,10 @@ impl<EK: KvEngine, ER: RaftEngine> Peer<EK, ER> {
                    }
                }
                AdminCmdType::CompactLog => self.propose_compact_log(ctx, req),
+                AdminCmdType::UpdateGcPeer => {


If I understand it correctly, we rely on this cmd to propagate the removed peer list.
But what if a conf change happen and a newly added node become leader. And at that time how does the new node get the removed node list? (maybe this cmd not run yet before that node get elected).
My original thought is this information needs to be transferred from the snapshot.

The list is part of region local state and snapshot state. Every node should have the exact same peer list when they applies to the same index.

The protocol updates can be found in pingcap/kvproto#1040.

The protocol updates can be found in pingcap/kvproto#1040.

I see. So it's part of snapshot. Thanks.

tabokie · 2023-01-31T06:47:54Z

components/raftstore-v2/src/operation/ready/mod.rs

+            if write_task.messages.is_empty() {
+                write_task.messages = self.take_pending_messages();
+            } else {
+                write_task.messages.extend(self.take_pending_messages());


append is better.

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

BusyJay · 2023-02-03T04:37:37Z

/merge

ti-chi-bot · 2023-02-03T04:37:38Z

@BusyJay: 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 · 2023-02-03T04:37:40Z

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

Commit hash: b227f9f

BusyJay · 2023-02-03T05:07:43Z

/test

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

BusyJay · 2023-02-03T06:04:55Z

/merge

ti-chi-bot · 2023-02-03T06:04:56Z

@BusyJay: 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 · 2023-02-03T06:04:58Z

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

Commit hash: 45bd91c

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

BusyJay · 2023-02-03T09:12:34Z

/test

BusyJay · 2023-02-03T09:12:48Z

/merge

ti-chi-bot · 2023-02-03T09:12:49Z

@BusyJay: 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 · 2023-02-03T09:12:51Z

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

Commit hash: df107c5

ti-chi-bot · 2023-02-03T09:13:05Z

@BusyJay: 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.

BusyJay · 2023-02-03T09:57:42Z

/test

BusyJay · 2023-02-03T10:30:30Z

/test

tabokie · 2023-02-24T14:07:16Z

components/raftstore-v2/src/operation/life.rs

+    // No need to set epoch as we don't know what it is.
+    tombstone_msg
+        .mut_extra_msg()
+        .set_type(ExtraMessageType::MsgGcPeerRequest);


I think this should be MsgGcPeerResponse.

No, it should be MsgGcPeerRequest. As the function name and comment says, the request is forwarded to source peer.

Forget it, I misread.

What is confusing though, is the use of MsgGcPeerRequest, when in fact only the set_is_tombstone is needed. For a reader reading MsgGcPeerRequest, he naturally wants to know the handling of it by skipping to on_gc_peer_request, but that function only makes sense for target peer not source peer.

MsgGcPeerRequest is necessary. is_tombstone is used by regular destroy, which indicates from is in the same region as to. MsgGcPeerRequest is for merge destroy, from may not be in the same region as to.

BusyJay requested review from tonyxuqqi, Connor1996 and tabokie January 16, 2023 07:29

ti-chi-bot added release-note-none size/XXL labels Jan 16, 2023

fix copr

ef3a2bb

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

ti-chi-bot added the needs-rebase label Jan 19, 2023

Merge branch 'master' into introduce-gc-peers

e70ea42

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

ti-chi-bot removed the needs-rebase label Jan 29, 2023

fix test

68290d5

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

tabokie reviewed Jan 30, 2023

View reviewed changes

address comment

84f0957

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

tonyxuqqi reviewed Jan 31, 2023

View reviewed changes

tabokie reviewed Jan 31, 2023

View reviewed changes

tabokie approved these changes Jan 31, 2023

View reviewed changes

ti-chi-bot added status/LGT1 Status: PR - There is already 1 approval needs-rebase labels Jan 31, 2023

BusyJay added 2 commits January 31, 2023 15:16

address comment

7c0246c

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

Merge branch 'master' into introduce-gc-peers

b227f9f

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

ti-chi-bot removed the needs-rebase label Feb 2, 2023

tonyxuqqi approved these changes Feb 2, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Status: PR - There are already 2 approvals and removed status/LGT1 Status: PR - There is already 1 approval labels Feb 2, 2023

ti-chi-bot added the status/can-merge Status: Can merge to base branch label Feb 3, 2023

Merge branch 'master' into introduce-gc-peers

a42a587

ti-chi-bot added the needs-rebase label Feb 3, 2023

Merge branch 'master' into introduce-gc-peers

45bd91c

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

ti-chi-bot removed status/can-merge Status: Can merge to base branch needs-rebase labels Feb 3, 2023

ti-chi-bot added the status/can-merge Status: Can merge to base branch label Feb 3, 2023

fix clippy

df107c5

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

ti-chi-bot removed the status/can-merge Status: Can merge to base branch label Feb 3, 2023

ti-chi-bot added the status/can-merge Status: Can merge to base branch label Feb 3, 2023

Merge branch 'master' into introduce-gc-peers

a5b9ef9

ti-chi-bot merged commit 6daed4f into tikv:master Feb 3, 2023

ti-chi-bot added this to the Pool milestone Feb 3, 2023

BusyJay deleted the introduce-gc-peers branch February 3, 2023 10:59

tabokie reviewed Feb 24, 2023

View reviewed changes

raftstore-v2: support tracing peer lifetime #14056

raftstore-v2: support tracing peer lifetime #14056

Conversation

BusyJay commented Jan 16, 2023

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jan 16, 2023 • edited

BusyJay commented Jan 29, 2023

overvenus commented Jan 29, 2023

BusyJay commented Jan 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BusyJay commented Feb 3, 2023

ti-chi-bot commented Feb 3, 2023

ti-chi-bot commented Feb 3, 2023

BusyJay commented Feb 3, 2023

BusyJay commented Feb 3, 2023

ti-chi-bot commented Feb 3, 2023

ti-chi-bot commented Feb 3, 2023

BusyJay commented Feb 3, 2023

BusyJay commented Feb 3, 2023

ti-chi-bot commented Feb 3, 2023

ti-chi-bot commented Feb 3, 2023

ti-chi-bot commented Feb 3, 2023

BusyJay commented Feb 3, 2023

BusyJay commented Feb 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot commented Jan 16, 2023 •

edited