Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/24630 )
Change subject: [tools/master] make move of a leader replica recoverable ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/24630/2/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc: http://gerrit.cloudera.org:8080/#/c/24630/2/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1340 PS2, Line 1340: ASSERT_OK(GetConsensusState(observer, tablet_id_, kTimeout, > nit: the disabled-case relies on a fixed SleepFor(5s) to assert the master Good point. Following the same pattern used in raft_consensus-itest.cc, I've wrapped the sleep and the "never evicts" assertions in AllowSlowTests(). http://gerrit.cloudera.org:8080/#/c/24630/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/24630/2/src/kudu/master/catalog_manager.cc@5517 PS2, Line 5517: bool AsyncLeaderStepDownForReplacementTask::SendRequest(int attempt) { > q: If the 'replace'-marked leader steps down but is not yet evicted, nothin Yes, I think that's intended and safe and that's not a real ping-pong. 1. While it's leader, a replace-marked replica can't self-elect — BecomeLeaderUnlocked() calls DisableFailureDetector() . 2. The step-down is GRACEFUL, i.e. TransferLeadership() hands off to a caught-up voter; on granting its vote the ex-leader snoozes its own failure detector (RequestVoteRespondVoteGranted, raft_consensus.cc:2454-2466), so it won't immediately re-run and the new leader gets a window to evict it. 3. If the transfer never completes, it simply stays leader (never stepped down) and the master just retries on the next report — no ping-pong there either. So the worst case is a bounded number of idempotent step-down RPCs. http://gerrit.cloudera.org:8080/#/c/24630/2/src/kudu/master/catalog_manager.cc@5900 PS2, Line 5900: rpcs.emplace_back(new AsyncLeaderStepDownForReplacementTask( > q: Unlike the AsyncEvictReplicaTask / AsyncAddReplicaTask branches, a leade Good catch. But the churn is already bounded: before dispatch, ProcessTabletReport() dedups queued RPCs by (tablet_id, description) via ContainsTask() (catalog_manager.cc:6008), and a task stays in pending_tasks_ for its whole retry lifetime. The step-down task's description includes the tablet id and leader uuid (stable during the in-flight transfer), so repeated reports just hit "Not sending duplicate request". So an extra gate would be largely redundant. http://gerrit.cloudera.org:8080/#/c/24630/2/src/kudu/tools/tool_replica_util.cc File src/kudu/tools/tool_replica_util.cc: http://gerrit.cloudera.org:8080/#/c/24630/2/src/kudu/tools/tool_replica_util.cc@638 PS2, Line 638: > nit: IsReplicaMoveScheduled() runs GetTabletLeader()/GetConsensusState() wi You're right. We considered adding a bounded retry around the check, but decided against it: it just introduces another hard-coded 5s magic number (on top of the existing ksck one) to paper over a rare, transient window. Since move_replica is a manual command and is now idempotent, letting the operator just re-run it is the cleaner option. http://gerrit.cloudera.org:8080/#/c/24630/2/src/kudu/tools/tool_replica_util.cc@668 PS2, Line 668: (!from_ts_uuid_in_config || from_ts_uuid_has_replace); > q: In the 3-4-3 branch, `scheduled` becomes true when `to` is present and ` Good observation. But validating that from was a genuine replica doesn't really work here, because two different situations produce the same config state, and cstate carries no marker to tell them apart: 1. The legitimate idempotent case: a move that already completed — to was added and promoted, and from was evicted, so it's naturally gone from the config. This is exactly the state the check is meant to recognize so a re-run resumes and reports completion. 2. The bad-input case you describe: from was never a replica, and to happens to already be one. Given that it needs compounded invalid input (bogus from + to already a voter) and the outcome is a harmless misleading "success" with no config change, I'd treat it as out of scope. -- To view, visit http://gerrit.cloudera.org:8080/24630 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7b28ca4e41355bd82c76d50e137f06197b402bc Gerrit-Change-Number: 24630 Gerrit-PatchSet: 3 Gerrit-Owner: Yifan Zhang <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Comment-Date: Fri, 31 Jul 2026 11:15:48 +0000 Gerrit-HasComments: Yes
