bitflicker64 commented on code in PR #3185:
URL: https://github.com/apache/hugegraph/pull/3185#discussion_r3915218299


##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java:
##########
@@ -203,7 +204,67 @@ 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() {
+        Node node = this.raftNode;

Review Comment:
   Fixed in 5bd1b96. `raftNode` is now `private volatile Node raftNode`, and 
`isReady()` passes its snapshot to a private static `hasLeader(Node)` that the 
public `hasLeader()` also delegates to, so the field is read once per call. 
Readiness unit tests still pass on JDK 11.



##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/rest/StoreAPI.java:
##########
@@ -386,4 +396,30 @@ class StoreStatistics {
     public Serializable checkHealthy() {
         return "";
     }
+
+    /**
+     * Check Service Readiness
+     * Answers 200 only when this PD is part of a raft quorum, that is, the 
raft node is active
+     * and knows the current leader. Otherwise answers 503 so that anything 
gating on PD
+     * (a Store waiting to register, the Server's wait-storage.sh, a 
Kubernetes readiness probe)

Review Comment:
   Fixed in 5bd1b96 with the suggested wording. The javadoc now names the 
compose healthcheck in front of Stores and Kubernetes readiness probes as the 
consumers, since `wait-storage.sh` polls `/v1/stores` and Stores register over 
gRPC.



##########
hugegraph-pd/docs/api-reference.md:
##########
@@ -774,12 +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",
+  "leader": "192.168.1.1:8610",
+  "isLeader": true
+}
+```
+
+A follower reports `"state": "STATE_FOLLOWER"` with the leader's raft address.
+When the quorum is lost the PD keeps answering `/v1/health` with `200` but
+`/v1/ready` turns into `503` with `"ready": false` and `"leader": null`.
+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.
+
 ### Metrics
 
 ```bash
 curl http://localhost:8620/actuator/metrics
 ```
 
+Raft membership gauges (Prometheus names, scraped from `/actuator/prometheus`)
+for alerting on quorum loss:
+
+| Gauge | Value |
+|-------|-------|
+| `hg_raft_leader` | `1` on the raft leader, `0` elsewhere |
+| `hg_raft_has_leader` | `1` while this PD sees a leader (is inside a quorum), 
`0` otherwise |
+| `hg_raft_alive_peers` | On the leader, the number of peers (itself included) 
heard from within the election timeout; `NaN` on other nodes |
+
+A cluster has lost its quorum when `sum(hg_raft_leader) == 0` or when
+`hg_raft_has_leader == 0` on every member.

Review Comment:
   Fixed in 5bd1b96. Took the suggested paragraph about the `for:` clause, and 
moved the whole gauge block under a new "Raft membership gauges" heading after 
the existing `/actuator/metrics` response example, so the `curl` fence and its 
response stay together and the `pd_*` example no longer reads as output for the 
`hg_*` table.



##########
docker/README.md:
##########
@@ -202,6 +202,14 @@ 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 leader and
+`503` otherwise, so the compose healthchecks gate Stores on `/v1/ready`. A
+single PD elects itself; three PDs become ready once two can talk to each
+other.

Review Comment:
   Fixed in 5bd1b96. The paragraph now says `/v1/ready` first ships in 1.8.0, 
spells out the failure mode with an older `HUGEGRAPH_VERSION` (PD healthcheck 
never passes, Stores never start), and points at pinning 1.8.0 or newer or 
building from source with `docker-compose.dev.yml`. Doubled blank line removed.



-- 
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]

Reply via email to