bitflicker64 commented on code in PR #3185:
URL: https://github.com/apache/hugegraph/pull/3185#discussion_r3923360619
##########
docker/docker-compose-hstore.yml:
##########
@@ -44,7 +44,7 @@ services:
volumes:
- pd-data:/hugegraph-pd/pd_data
healthcheck:
- test: ["CMD-SHELL", "curl -fsS http://localhost:8620/v1/health
>/dev/null"]
+ test: ["CMD-SHELL", "curl -fsS http://localhost:8620/v1/ready
>/dev/null"]
Review Comment:
Reverted in c8adc85: both compose files are byte-identical to master again,
and the manual verification calls in the README go back to `/v1/health`.
Reproduced your finding locally against a source-built PD: an auth-gated
path and a path that does not exist both answer `200` with
`{"error":"Unauthorized!","status":-1}`, and `curl -fsS` exits 0 on it. The
payload grep rejects it.
I took the second option rather than pinning smoke to source-built images,
because the hardened probe would also hang the documented quickstart for anyone
on published images until the endpoint ships. The README now records the body
match and the image requirement for whoever switches them over.
Root cause is `RestAuthentication.preHandle` never calling `setStatus`. Out
of scope here; happy to file it separately.
##########
docker/README.md:
##########
@@ -202,6 +202,15 @@ 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. `/v1/ready` first ships in 1.8.0: with an older `HUGEGRAPH_VERSION`
+the PD healthcheck never passes and the Stores never start, so pin 1.8.0
+or newer, or build the images from source with `docker-compose.dev.yml`.
Review Comment:
Rewritten in c8adc85. The paragraph no longer claims a hang. It now says the
healthchecks stay on `/v1/health` because these files run published images, and
lists the two things needed before switching: match on the body with `grep -q
'"ready":true'` (with the reason, since PD answers `200` and an error envelope
on any non-excluded path), and an image that carries the endpoint.
Lines 134 and 189 are back on `/v1/health`. Dropped the "1.8.0" guess for
"from the next release onwards".
##########
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/rest/RestApiTest.java:
##########
@@ -62,6 +62,51 @@ public void testQueryClusterInfo() throws
URISyntaxException, IOException, Inter
assert obj.getInt("status") == 0;
}
+ @Test
+ public void testHealthNeedsNoAuth() throws URISyntaxException, IOException,
+ InterruptedException {
+ String url = pdRestAddr + "/v1/health";
+ HttpRequest request = HttpRequest.newBuilder().uri(new
URI(url)).GET().build();
+ HttpResponse<String> response = client.send(request,
HttpResponse.BodyHandlers.ofString());
+ assert response.statusCode() == 200;
Review Comment:
Fixed in c8adc85 with the suggested assertion, plus a comment recording why
the status alone is not enough. Verified against a live PD that `/v1/health`
returns an empty body.
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/metrics/PDMetrics.java:
##########
@@ -76,7 +77,28 @@ private void registerMeters() {
Gauge.builder(PREFIX + ".terms", () -> setTerms())
.description("term of partitions in PD")
.register(registry);
+ registerRaftMeters();
+ }
+ /**
+ * Raft membership gauges so operators can alert on quorum loss. They
mirror what
+ * {@code GET /v1/ready} answers: a PD that sees no leader is outside a
quorum.
+ */
+ private void registerRaftMeters() {
+ RaftEngine raft = RaftEngine.getInstance();
+ Gauge.builder(PREFIX + ".raft.leader", () -> raft.isLeader() ? 1 : 0)
+ .description("1 if this PD is the raft leader, 0 otherwise")
+ .register(registry);
+ Gauge.builder(PREFIX + ".raft.has_leader", () -> raft.hasLeader() ? 1
: 0)
+ .description("1 if this PD sees a raft leader, i.e. is part of a
quorum, 0 otherwise")
+ .register(registry);
+ Gauge.builder(PREFIX + ".raft.alive_peers", () -> {
+ int alive = raft.getAlivePeerCount();
+ return alive < 0 ? Double.NaN : alive;
+ })
+ .description("Number of raft peers, itself included, the leader
has heard from " +
+ "within the election timeout; NaN on non-leader
nodes")
Review Comment:
Fixed in c8adc85 with the suggested wording. Confirmed against jraft-core
1.3.13: `NodeOptions.getLeaderLeaseTimeoutMs()` is `electionTimeoutMs *
leaderLeaseTimeRatio / 100` and the constructor sets the ratio to 90, which
`RaftEngine.init` does not override.
Also corrected in `RaftEngine.getAlivePeerCount()` and in
`api-reference.md`, both of which now say leader lease timeout and name the 90%
default so operators can size alert windows off it.
--
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]
