bitflicker64 commented on code in PR #3185: URL: https://github.com/apache/hugegraph/pull/3185#discussion_r3941071813
########## hugegraph-pd/docs/api-reference.md: ########## @@ -774,6 +774,54 @@ curl http://localhost:8620/actuator/health } ``` +### Liveness and Readiness + +Two unauthenticated endpoints are meant for probes and startup gates: + +| Endpoint | Meaning | Status | +|----------|---------|--------| +| `GET /v1/health` | Liveness: the REST listener is up. Does not consult raft. | always `200` | +| `GET /v1/ready` | Readiness: the raft node is active and sees a leader, so this PD is inside a quorum. | `200` when ready, `503` otherwise | + +```bash +curl -i http://localhost:8620/v1/ready +``` + +**Response** (leader of a healthy cluster): +```json +{ + "ready": true, + "state": "STATE_LEADER", + "isLeader": true +} +``` + +A follower reports `"state": "STATE_FOLLOWER"` with `"isLeader": false`. When +the quorum is lost the PD keeps answering `/v1/health` with `200` but +`/v1/ready` turns into `503` with `"ready": false`. Being unauthenticated, the +body carries no cluster addresses; the leader's address stays on `/v1/members`. + +The answer is served from state the raft callbacks maintain rather than from +the raft node, so it stays prompt while an election is running and never waits +on the node lock. `state` is therefore the last change raft announced: jraft +emits no callback for candidacy or leadership transfer, so a candidate reports +`STATE_FOLLOWER` with `"ready": false`. Review Comment: 🧹 `STATE_UNINITIALIZED` is absent from this section, and it is what a PD reports for the whole window the endpoint exists to cover. `RaftStateMachine.probeState` starts at `State.STATE_UNINITIALIZED` (`RaftStateMachine.java:63`) and is only ever written by a callback, so a PD whose raft node is up but has never seen a leader answers `"state": "STATE_UNINITIALIZED"`. `RaftEngineReadinessTest.testStartedNodeWithoutAnyCallbackIsNotReady` in this PR asserts that `ready` is false and `state` is `STATE_UNINITIALIZED` for exactly that case. That is the ordinary pre-quorum startup `docker/README.md` describes ("three PDs become ready once two of them can talk to each other"), yet this section names only `STATE_LEADER` and `STATE_FOLLOWER`, and the sentence above it covers a candidate that had already been following a leader, not a node that has never seen one. `RaftEngine.getRaftStatus()` returns the same string on its `raftNode == null` branch (`RaftEngine.java:236-237`), so the body cannot separate "raft never started" from "raft running, no quorum yet". Requested change: name `STATE_UNINITIALIZED` here as the state a PD reports until its first raft callback. If the two cases are worth telling apart, given that `state` was kept in the anonymous body because it "is what makes a `503` diagnosable", returning `this.stateMachine.getProbeState().name()` on the `raftNode == null` branch would leave `STATE_UNINITIALIZED` for a node that never started. ########## hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java: ########## @@ -203,7 +204,94 @@ public void shutDown() { } public boolean isLeader() { - return this.raftNode.isLeader(true); + Node node = this.raftNode; + return node != null && node.isLeader(true); + } + + /** + * Whether this node currently sees a raft leader. + * <p> + * A follower only keeps its leader while heartbeats keep arriving inside the election + * timeout, and a leader only keeps its role while it can reach a quorum. Seeing a leader + * therefore means this node is part of a quorum from its own point of view, which is the + * signal a readiness probe needs. Served from the state machine callbacks, not from the + * raft node, so it never waits on the node lock. + */ + public boolean hasLeader() { + return this.raftNode != null && this.stateMachine.seesLeader(); + } + + /** + * Take a view of the local raft state from the volatile copies the state machine + * callbacks maintain, never from the raft node itself. During an election jraft holds + * the node lock while it reconnects to peers, so a probe that read the node stalled for + * the connect timeout instead of answering its 503 promptly. All fields derive from the + * same callback-written values, so they cannot contradict each other. + * <p> + * The state reported is the last one a callback announced: leader, follower, error or + * shutdown. jraft emits no callback for candidacy or leadership transfer, so a candidate + * reads as a follower that sees no leader, which yields the same not-ready answer. + */ + public RaftStatus getRaftStatus() { + if (this.raftNode == null) { + return new RaftStatus(false, State.STATE_UNINITIALIZED.name(), false); + } + State state = this.stateMachine.getProbeState(); + return new RaftStatus(state.isActive() && this.stateMachine.seesLeader(), Review Comment: 🧹 The javadoc at L229 says "All fields derive from the same callback-written values, so they cannot contradict each other." This method makes two independent volatile reads: `getProbeState()` on L239 and `seesLeader()` here. `RaftStateMachine.onLeaderStart` writes `probeState = STATE_LEADER` and then `seesLeader = true` as two separate stores (`RaftStateMachine.java:144-145`). A reader landing between them answers `{"ready":false,"state":"STATE_LEADER","isLeader":true}`, which is the contradiction the javadoc rules out. `onLeaderStop` (L159-160) writes the same pair in the same order, so the mirror case, `ready:true` for a node that has already stopped being leader, is reachable too. Two reasons to close it rather than live with it. It is the same overpromise raised on the `Node`-based version and answered in 7074440, reintroduced in a new form when the probe moved onto the callbacks. And the staleness is wider than these two stores: `FSMCallerImpl` enqueues `LEADER_START`, `LEADER_STOP`, `START_FOLLOWING` and `STOP_FOLLOWING` onto the FSM disruptor (jraft-core 1.3.13, `FSMCallerImpl.java:291-320`), so both fields trail the node by however long the already-queued `onApply` batches take. Requested change: hold the state and the leader flag in one volatile immutable object in `RaftStateMachine`, written once per callback, and read that field once in `getRaftStatus()` and `hasLeader()`. Same single-snapshot shape 7074440 applied to the previous version. -- 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]
