[imbajin]

Before → After

Before: a membership-change retry could treat existing learners as completed additions and return OK without promoting them after a timeout.

After: distinguish newly added learners from target members still awaiting promotion, resume catch-up/promotion on retry, and clean up stale learners while retaining node-destruction ordering.

Validation boundary

The source PR reports 13 regression tests invoking the production method with simulated Node/RPC boundaries, including timeout/retry and partial promotion; the original implementation failed nine of the first 11 cases. These tests do not establish independent multi-node failure/recovery acceptance.

Source PR: 🔗 hugegraph/hugegraph #342

Visual overview

Retries distinguish absent members, existing learners and voters; existing learners still catch up and attempt promotion. This diagram explains the code path, not real-cluster fault or RC acceptance.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.41%. Comparing base (608cb1d) to head (cdce746).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3419      +/-   ##
============================================
+ Coverage     57.27%   57.41%   +0.14%     
+ Complexity     6577     2422    -4155     
============================================
  Files           776      776              
  Lines         68881    68883       +2     
  Branches       9431     9433       +2     
============================================
+ Hits          39449    39547      +98     
+ Misses        25804    25702     -102     
- Partials       3628     3634       +6     

Flag	Coverage Δ		*Carryforward flag
commons	96.37% <ø> (ø)		Carriedforward from 5629712
hstore	36.09% <ø> (+<0.01%)	⬆️
pd	52.17% <ø> (+<0.01%)	⬆️	Carriedforward from 5629712
server_hbase	42.18% <ø> (ø)		Carriedforward from 5629712
server_memory	55.98% <ø> (ø)		Carriedforward from 5629712
server_rocksdb	42.65% <ø> (ø)		Carriedforward from 5629712
store	33.81% <100.00%> (+0.66%)	⬆️

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bitflicker64]

Review summary (head cdce746)

Score: 10/10. No defect introduced by this PR was confirmed.

Coverage: 4 of 4 independent whole-PR reviews received; none pending. None of the four found a defect introduced by this PR. All four changed files were covered (PartitionEngine.java, PartitionEngineMembershipTest.java, hg-store-test/pom.xml, docs/operations-guide.md), diffed against merge-base 9a8f066.

What was checked in PartitionEngine.changePeers:

• Fresh add: pendingPromotions equals addPeers when no target learner exists yet, so behavior matches the base.
• Retry after catch-up timeout: target learners that already exist are waited on and promoted without recreating their raft nodes. In the base, this retry returned OK and left them as learners. Covered by testCatchupTimeoutResumesWithoutRecreatingLearners and testResumesExistingLearnersWithoutRecreatingNodes.
• Voters already match but stale learners remain: the base returned OK and kept them. The head no longer returns early, so the existing removal path removes and destroys them (testRemovesStaleLearnerEvenWhenVotersAlreadyMatch).
• removeLearners now receives only the peers being promoted, not every learner in the conf. After a partial promotion (learners removed, then changePeers failed), the retry re-adds those peers through the add path, which recreates their raft nodes (testRetriesAfterLearnersWereRemovedButPromotionFailed).
• The log.error calls now print the status that actually failed. The base printed the earlier, successful status.

Unchanged from base (not scored): if removeLearners succeeds and the follow-up changePeers fails, the method returns TASK_ERROR. doChangeShard re-enqueues only on TASK_CONTINUE, so recovery waits for PD to issue a new task, as it did before.

Accepted trade-off of the fix (documented, not scored): each retry now waits up to 600s again and returns TASK_CONTINUE. A learner that never catches up keeps re-enqueuing the sync task with no overall deadline. The operations guide now describes this.

Limits: the new tests mock the JRaft Node, override waitForReplicate and call changePeers directly. Neither the doChangeShard retry loop nor a real multi-node failure and recovery was exercised. Source was read at the pinned base and head and against the JRaft 1.3.14 sources.

Local checks at head cdce746 (one review): mvn install -pl hugegraph-store/hg-store-test -am -DskipTests succeeded, and PartitionEngineMembershipTest plus PartitionEngineErrorTest passed 17/17. With the base PartitionEngine.java restored (only waitForReplicate made package-private so the tests compile), 11 of the 13 new tests fail, so they do detect the original bug. PR CI: 10 checks passed, 7 skipped.