bitflicker64 commented on code in PR #3185:
URL: https://github.com/apache/hugegraph/pull/3185#discussion_r3930889346
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java:
##########
@@ -203,7 +204,105 @@ 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 knows a raft leader.
+ * <p>
+ * A follower only keeps its leader id while heartbeats keep arriving
inside the election
+ * timeout, and a leader only keeps its role while it can reach a quorum.
A non-null leader
+ * therefore means this node is part of a quorum from its own point of
view, which is the
+ * signal a readiness probe needs.
+ */
+ public boolean hasLeader() {
+ return hasLeader(this.raftNode);
+ }
+
+ private static boolean hasLeader(Node node) {
+ if (node == null) {
+ return false;
+ }
+ PeerId leader = node.getLeaderId();
+ return leader != null && !leader.isEmpty();
+ }
+
+ /**
+ * Whether this node can take part in serving requests: the raft node has
been started,
+ * is in an active state (leader, follower or transferring leadership) and
sees a leader.
Review Comment:
🧹 The list of active states is short by one. `isReady()` gates on
`State.isActive()`, which jraft-core 1.3.13 defines as `this.ordinal() <
STATE_ERROR.ordinal()` over `STATE_LEADER, STATE_TRANSFERRING, STATE_CANDIDATE,
STATE_FOLLOWER, STATE_ERROR, ...`, so a candidate is active too.
Nothing is broken: `NodeImpl.handleElectionTimeout` calls
`resetLeaderId(PeerId.emptyPeer(), ...)` before `preVote()`, so a candidate has
no leader id left. But that means `testCandidateIsNotReady` passes on its
`null` stub, not on the state check. Same shape at line 228:
`NodeImpl.getLeaderId()` returns `this.leaderId.isEmpty() ? null :
this.leaderId`, so `testEmptyLeaderIdIsNotReady` exercises a value a real node
never returns.
Requested change: name the real set here, and either rename
`testCandidateIsNotReady` to `testCandidateWithoutLeaderIsNotReady` or add a
`STATE_CANDIDATE` plus leader-id case that records what the code actually does.
```suggestion
* is in an active state, which jraft's {@code State.isActive()} takes
to mean leader,
* transferring, candidate or follower, and sees a leader.
```
##########
docker/README.md:
##########
@@ -202,6 +202,24 @@ done
curl -fsS http://localhost:8088/about
```
+PD answers two unauthenticated probe endpoints. `/v1/health` is liveness only:
+it returns `200` as soon as the REST listener is up, even when the PD has no
+raft leader. `/v1/ready` returns `200` only while the PD sees a raft leader,
+and `503` otherwise. A single PD elects itself; three PDs become ready once
+two of them can talk to each other.
+
+The healthchecks in these files still gate on `/v1/health`, because
Review Comment:
🧹 One PD probe in the repository sits outside this reasoning.
`hugegraph-pd/Dockerfile:68-69` bakes `HEALTHCHECK ... CMD curl -fsS
http://localhost:8620/v1/health >/dev/null` into the image, and that image is
built from this tree, so it always carries `/v1/ready`. The version-skew
argument for keeping the compose files on liveness does not apply to a probe
shipped inside the image.
Both compose files override it (`docker/docker-compose-hstore.yml:47`,
`docker/docker-compose-3pd-3store-3server.yml:40`), so it only governs `docker
run` and orchestration that inherits the image probe. Those users still read a
quorum-less PD as healthy.
Requested change: add a sentence here saying the image's own HEALTHCHECK
stays on liveness too. Changing the Dockerfile is outside this diff; worth a
follow-up issue.
##########
hugegraph-pd/docs/api-reference.md:
##########
@@ -774,6 +774,46 @@ 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`.
+
+Point Kubernetes readiness probes, `depends_on` healthchecks and any
+"wait for PD" script at `/v1/ready`; keep liveness probes on `/v1/health`
+so a PD that merely lost its leader is not restarted.
+
+Match on the body rather than on the status code alone. PD's auth interceptor
+rejects a request it does not exclude by writing an error envelope without
Review Comment:
🧹 Nit, and a coordination one. This states current interceptor behaviour as
a property of PD, and open PR #3189 (`fix(pd): validate REST credentials and
return 401 on refusal`, head 5c339c1) changes exactly it: it adds
`response.setStatus(HttpServletResponse.SC_UNAUTHORIZED)` and a
`WWW-Authenticate` header to the same catch block in
`RestAuthentication.preHandle`. On a PD carrying that, a rejected request
answers `401`, so the reason given here and the sentence about a status-only
probe reading an older PD as ready stop holding. The body-match advice itself
survives.
The same claim appears at `docker/README.md:215-218` and inside
`RestApiTest.testHealthNeedsNoAuth` (`RestApiTest.java:72-73`). Whichever PR
lands second leaves three stale copies, so it is worth attributing rather than
asserting, in all three.
--
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]
