bitflicker64 commented on code in PR #3185:
URL: https://github.com/apache/hugegraph/pull/3185#discussion_r3915168039
##########
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:
🧹 `raftNode` (line 73) is a plain field. It is written inside `synchronized
init(...)`, and read without synchronization here and in `isReady()`,
`getNodeState()`, `isLeader()`, `getLeader()` and `getAlivePeerCount()`.
Publication is safe today, but only incidentally: all three callers of
`init(...)` are `@PostConstruct` (`PDService:179`, `DiscoveryService:59`,
`KvServiceGrpcImpl:75`), so the write happens during context refresh, before
the Tomcat connector accepts and before the gRPC server starts. The null checks
this PR adds are now the contract behind `/v1/ready` and the `hg.raft.*`
gauges, both read from threads that never touch the write lock, so it is worth
not depending on that ordering.
Requested change: declare the field `private volatile Node raftNode;` at
line 73.
While here: `isReady()` snapshots the field into `node`, then delegates to
`hasLeader()`, which reads it a second time. Harmless, but a private
`hasLeader(Node)` overload would keep the snapshot idiom consistent.
##########
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:
🧹 `wait-storage.sh` does not gate on this endpoint. It polls
`http://${peer}/v1/stores` and greps for `"state":"Up"`
(`hugegraph-server/hugegraph-dist/src/assembly/static/bin/wait-storage.sh:104-110`),
and this PR does not change it. The Store itself registers over gRPC, so the
only thing actually held back by `/v1/ready` today is the compose healthcheck.
Requested change: drop the `wait-storage.sh` reference.
```suggestion
* (the compose healthcheck in front of Stores, a Kubernetes readiness
probe)
```
##########
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:
🧹 Both expressions are transiently true during any normal leader change,
such as a leader restart or a rolling upgrade, because `hg_raft_leader` is 0
everywhere and every member resets its leader id for the duration of the
election. An alert written on the instantaneous value therefore pages on
healthy behaviour.
```suggestion
A cluster has lost its quorum when `sum(hg_raft_leader) == 0` or when
`hg_raft_has_leader == 0` on every member. Both are briefly true during a
normal election, so alert on them with a `for:` clause longer than the
election timeout rather than on the instantaneous value.
```
Separately, placement: this block lands between the `curl
http://localhost:8620/actuator/metrics` fence at line 810 and its `**Response**
(Prometheus format):` example at line 825, so that example now reads as the
response to the new gauge paragraph. It still shows `pd_raft_state` and
`pd_store_count`, which contradicts the `hg_` names the table just introduced.
Moving the new content below line 839 keeps the existing command and response
together.
##########
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:
🧹 Worth naming the first release that ships `/v1/ready` in this paragraph.
The compose files pull `hugegraph/pd:${HUGEGRAPH_VERSION:-latest}` and line 239
documents pinning (`HUGEGRAPH_VERSION=1.7.0`), so pointing the new compose file
at an older PD image means `/v1/ready` does not exist, `curl -fsS` fails, the
PD never turns healthy, and `store` (`depends_on: condition: service_healthy`)
never starts. That is a silent hang rather than a clear error, and
`docker-compose-3pd-3store-3server.yml:40` has the same exposure.
Also, lines 211-212 are two consecutive blank lines; one is enough.
--
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]
