bitflicker64 commented on code in PR #3358:
URL: https://github.com/apache/hugegraph/pull/3358#discussion_r4306903585
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftStateMachine.java:
##########
@@ -214,24 +217,29 @@ 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 (!isTerminal()) {
+ this.probeView = new ProbeView(State.STATE_FOLLOWER, false);
+ }
super.onLeaderStop(status);
log.info("Raft lost leader ");
}
@Override
public void onStartFollowing(final LeaderChangeContext ctx) {
+ if (isTerminal()) {
+ return;
+ }
this.probeView = new ProbeView(State.STATE_FOLLOWER, true);
super.onStartFollowing(ctx);
notifyStateListeners();
}
+ private boolean isTerminal() {
Review Comment:
Minor: `isTerminal()` makes `STATE_SHUTDOWN` permanent for this
`RaftStateMachine` object, but the object outlives the raft node. `RaftEngine`
creates it once in its constructor (`RaftEngine.java:88`), and `init()` hands
that same object to every new node (`nodeOptions.setFsm(stateMachine)`, line
122). `shutDown()` sets `raftNode` to null, so a later `init()` starts a new
node (the guard at line 96 passes), but `probeView` still holds
`STATE_SHUTDOWN` from the old one. On the new node `onLeaderStart` and
`onStartFollowing` return early. `leaderTerm` stays -1, `/v1/health` and
`/v1/ready` keep answering 503, and no state listener runs.
`PDCoreSuiteTest` hits this today. `PDCoreTestBase` calls `init()` on the
singleton in `@BeforeClass` and `shutDown()` in `@AfterClass`, so from
`IdServiceTest` onward a leader start no longer runs
`ConfigService.onRaftLeaderChanged` (`loadConfig()`) or
`PartitionService.onRaftLeaderChanged` (`partitionMeta.reload()`). Those suites
pass on a PD that reports itself shut down. A production PD does not restart
the engine inside one JVM, so this is a test-fidelity problem and a latent
trap, not a live bug.
Requested change: scope the terminal guard to the node that reported it.
Reset the probe view to `STATE_UNINITIALIZED` and `leaderTerm` to -1 in
`RaftEngine.init()` before `raftGroupService.start(false)`, for example through
a package-private `RaftStateMachine` reset method. Add a
`RaftEngineReadinessTest` case: after `onShutdown()` and the reset,
`onLeaderStart(7)` reports `STATE_LEADER`.
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/rest/StoreAPI.java:
##########
@@ -388,22 +388,20 @@ class StoreStatistics {
* This interface is used to check the health status of the service by
accessing the /health
* path via a GET request.
* <p>
- * This is a liveness signal only: it answers 200 as soon as the REST
listener is up and
- * does not consult the raft state. Use {@link #checkReady()} to find out
whether this PD
- * can actually serve.
+ * This liveness signal answers 503 for a fatal raft error or a stopped
state machine.
+ * Elections and temporary quorum loss remain healthy. Use {@link
#checkReady()} to find
+ * out whether this PD can actually serve.
*
* @return Returns a string indicating the service's health status.
Typically, an empty
* string indicates the service is healthy.
*/
- // TODO: answer non-200 when the raft state machine is in STATE_ERROR (a
failed snapshot) or
- // the KV store never opened (a held RocksDB LOCK): both leave this PD
unable to recover while
- // /health stays 200 forever. The Helm chart (helm/hugegraph) derives a
single PD's startup
- // and liveness probes to /v1/ready for this reason; drop that derivation
once this reports it.
- // https://github.com/apache/hugegraph/issues/3222
- // https://github.com/apache/hugegraph/issues/3226
@GetMapping(value = "/health", produces = MediaType.TEXT_PLAIN_VALUE)
- public Serializable checkHealthy() {
- return "";
+ public ResponseEntity<String> checkHealthy() {
Review Comment:
Minor: This hunk deletes the whole TODO, including the half for #3226, and
that issue is still open. #3226 covers a PD whose metadata RocksDB never opened
because the previous instance still holds its `LOCK`. That failure never
reaches a raft callback, and `checkHealthy()` reads only the raft probe view,
so such a PD still answers 200 here indefinitely. With the TODO gone, nothing
in the code points at #3226. `hugegraph-pd/docs/api-reference.md` now describes
`/v1/health` as "the REST listener is up and Raft has no terminal error", which
reads as a complete liveness check.
Requested change: keep a TODO above `checkHealthy()` for the store-open
case, with the #3226 link. In the api-reference row, or the paragraph after it,
say that a PD whose metadata store failed to open still answers 200 until #3226
is fixed.
--
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]
