imbajin commented on code in PR #3358:
URL: https://github.com/apache/hugegraph/pull/3358#discussion_r4297203450
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftStateMachine.java:
##########
@@ -214,13 +214,10 @@ public void onLeaderStart(final long term) {
@Override
public void onLeaderStop(final Status status) {
this.leaderTerm.set(-1);
- // TODO: keep STATE_ERROR set by onError instead of overwriting it
with STATE_FOLLOWER, so
- // the probe view, and with it /v1/ready, reports a PD that stepped
down for good after a
- // snapshot failure. /v1/health does not read this view; making it
report the state is the
- // separate TODO at StoreAPI.checkHealthy. The Helm chart
(helm/hugegraph) works around both
- // by deriving a single PD's startup and liveness probes to /v1/ready.
- // https://github.com/apache/hugegraph/issues/3222
- this.probeView = new ProbeView(State.STATE_FOLLOWER, false);
+ // JRaft steps down after reporting a fatal error. That callback does
not recover the node.
+ if (this.probeView.state != State.STATE_ERROR) {
Review Comment:
[P2] Keep the terminal error sticky across already-queued callbacks
This guard only protects onLeaderStop; onStartFollowing still
unconditionally replaces the probe with STATE_FOLLOWER. In the pinned JRaft
1.3.14 dependency,
[SnapshotExecutorImpl.reportError](https://github.com/sofastack/sofa-jraft/blob/v1.3.14/jraft-core/src/main/java/com/alipay/sofa/jraft/storage/snapshot/SnapshotExecutorImpl.java#L730-L733)
queues the FSM error before the node becomes terminal. A concurrent
higher-term AppendEntries can therefore queue LEADER_STOP and START_FOLLOWING
behind it while the node is still active. After ERROR calls this state
machine's onError and Node.onError, [the queued START_FOLLOWING is dispatched
without an error-status
guard](https://github.com/sofastack/sofa-jraft/blob/v1.3.14/jraft-core/src/main/java/com/alipay/sofa/jraft/core/FSMCallerImpl.java#L434-L444).
It overwrites STATE_ERROR; the subsequent onStopFollowing retains
STATE_FOLLOWER, so /v1/health returns 200 for the terminally failed node.
Please preserve terminal state across the follower/leader transition
callbacks, and add a regression for ERROR → LEADER_STOP → START_FOLLOWING →
STOP_FOLLOWING. Two independent source reviews confirmed this ordering. A
bounded Java 21 harness using the unchanged extracted callback bodies produced
STATE_ERROR/503 for the existing sequence and STATE_FOLLOWER/200 for the
queued-following sequence; dependencies were stubbed, so this is not a full
JRaft/PD race test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
