bitflicker64 commented on code in PR #3185:
URL: https://github.com/apache/hugegraph/pull/3185#discussion_r3952111034
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java:
##########
@@ -203,7 +204,97 @@ 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 sees a raft leader.
+ * <p>
+ * A follower only keeps its leader while heartbeats keep arriving inside
the election
+ * timeout, and a leader only keeps its role while it can reach a quorum.
Seeing a leader
+ * therefore means this node is part of a quorum from its own point of
view, which is the
+ * signal a readiness probe needs. Served from the state machine
callbacks, not from the
+ * raft node, so it never waits on the node lock.
+ */
+ public boolean hasLeader() {
+ return this.raftNode != null &&
this.stateMachine.getProbeView().seesLeader;
+ }
+
+ /**
+ * Take a view of the local raft state from the volatile copies the state
machine
+ * callbacks maintain, never from the raft node itself. During an election
jraft holds
+ * the node lock while it reconnects to peers, so a probe that read the
node stalled for
+ * the connect timeout instead of answering its 503 promptly. All fields
derive from one
+ * read of one immutable view, swapped in whole per callback, so they
cannot contradict
+ * each other.
+ * <p>
+ * The state reported is the last one a callback announced: leader,
follower, error or
+ * shutdown. jraft emits no callback for candidacy or leadership transfer,
so a candidate
+ * reads as a follower that sees no leader, which yields the same
not-ready answer. The
+ * view trails the node by whatever sits in the FSM queue ahead of the
announcement,
+ * which is the price of never waiting on the node lock.
+ */
+ public RaftStatus getRaftStatus() {
+ if (this.raftNode == null) {
+ return new RaftStatus(false, State.STATE_UNINITIALIZED.name(),
false);
+ }
+ RaftStateMachine.ProbeView view = this.stateMachine.getProbeView();
+ return new RaftStatus(view.state.isActive() && view.seesLeader,
+ view.state.name(), State.STATE_LEADER ==
view.state);
+ }
+
+ /**
+ * Immutable view of the raft state behind {@code GET /v1/ready}. It
carries no cluster
+ * addresses: the endpoint is unauthenticated, and the leader's address
stays on the
+ * authenticated {@code /v1/members}.
+ */
+ public static final class RaftStatus {
+
+ private final boolean ready;
+ private final String state;
+ private final boolean localLeader;
+
+ RaftStatus(boolean ready, String state, boolean localLeader) {
+ this.ready = ready;
+ this.state = state;
+ this.localLeader = localLeader;
+ }
+
+ public boolean isReady() {
+ return this.ready;
+ }
+
+ /**
+ * @return the jraft node state name, never null
+ */
+ public String getState() {
+ return this.state;
+ }
+
+ public boolean isLocalLeader() {
+ return this.localLeader;
+ }
+ }
+
+ /**
+ * Number of raft peers, this node included, that the leader has heard
from within the
+ * leader lease timeout, which jraft derives as 90% of the election
timeout by default.
+ * Only the leader tracks replication state, so any other node returns -1.
The leader
+ * check is the state machine's lock-free term flag, so a scrape on a
non-leader never
+ * waits on the node lock; {@code listAlivePeers} itself only runs on a
settled leader.
+ */
+ public int getAlivePeerCount() {
Review Comment:
Fixed in `2ac8f6c9e`. Checked it against the jraft 1.3.13 sources first and
the claim holds: `NodeImpl.listAlivePeers()` takes `readLock` before it checks
state, so no guard can keep a caller off the lock.
`getAlivePeerCount()` now returns a volatile field, published by a refresher
on its own daemon thread, and the javadoc says the value is the last published
one instead of promising a settled leader. The `-1` to `NaN` contract is
unchanged and is still what you get before the first refresh.
I did add a periodic refresh rather than a callback. jraft announces
leadership transitions but never announces a follower dropping out of the
replication set, so a callback-driven count would freeze at election-time
membership, which is wrong for exactly the quorum-loss case this gauge exists
for.
##########
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/rest/RestApiTest.java:
##########
@@ -62,6 +62,56 @@ 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;
+ // A 200 alone does not prove the path is anonymous: as of 1.7.0 the
auth interceptor
+ // refuses with 200 and an error envelope. checkHealthy() returns an
empty body, which
+ // separates the two whichever status a refusal carries.
+ assert response.body().isEmpty() : "expected an empty body, got " +
response.body();
Review Comment:
Fixed in `cf95f22ee`. The three methods this PR adds now use
`org.junit.Assert`, keeping the existing failure messages.
I left the rest of the file's bare asserts alone so the diff stays inside
what this PR introduces. Worth noting Surefire enables assertions by default,
so these did run in CI, but you are right that nothing guarantees it.
##########
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/rest/RestApiTest.java:
##########
@@ -62,6 +62,56 @@ 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;
+ // A 200 alone does not prove the path is anonymous: as of 1.7.0 the
auth interceptor
+ // refuses with 200 and an error envelope. checkHealthy() returns an
empty body, which
+ // separates the two whichever status a refusal carries.
+ assert response.body().isEmpty() : "expected an empty body, got " +
response.body();
+ }
+
+ @Test
+ public void testReadyNeedsNoAuthAndReflectsRaft() throws
URISyntaxException, IOException,
+
InterruptedException, JSONException {
+ // The CI PD is a single-node raft group, so it is its own leader and
must be ready
+ String url = pdRestAddr + "/v1/ready";
+ HttpRequest request = HttpRequest.newBuilder().uri(new
URI(url)).GET().build();
+ HttpResponse<String> response = client.send(request,
HttpResponse.BodyHandlers.ofString());
+ assert response.statusCode() == 200 : "expected 200, got " +
response.statusCode() +
+ " body=" + response.body();
+ JSONObject obj = new JSONObject(response.body());
+ assert obj.getBoolean("ready");
+ assert obj.getBoolean("isLeader");
+ assert "STATE_LEADER".equals(obj.getString("state"));
+ // Unauthenticated, so it must not disclose cluster addresses
+ assert !obj.has("leader") : "the anonymous body must not carry the
leader address";
Review Comment:
Fixed in `cf95f22ee`, same change as the sibling thread. `assert
!obj.has("leader") : msg` became `Assert.assertFalse(msg, ...)`, and the status
check became `Assert.assertEquals` with the body in the message, since
`assertEquals` already prints expected and actual.
##########
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/rest/RestApiTest.java:
##########
@@ -62,6 +62,56 @@ 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;
+ // A 200 alone does not prove the path is anonymous: as of 1.7.0 the
auth interceptor
+ // refuses with 200 and an error envelope. checkHealthy() returns an
empty body, which
+ // separates the two whichever status a refusal carries.
+ assert response.body().isEmpty() : "expected an empty body, got " +
response.body();
+ }
+
+ @Test
+ public void testReadyNeedsNoAuthAndReflectsRaft() throws
URISyntaxException, IOException,
+
InterruptedException, JSONException {
+ // The CI PD is a single-node raft group, so it is its own leader and
must be ready
+ String url = pdRestAddr + "/v1/ready";
+ HttpRequest request = HttpRequest.newBuilder().uri(new
URI(url)).GET().build();
+ HttpResponse<String> response = client.send(request,
HttpResponse.BodyHandlers.ofString());
+ assert response.statusCode() == 200 : "expected 200, got " +
response.statusCode() +
+ " body=" + response.body();
+ JSONObject obj = new JSONObject(response.body());
+ assert obj.getBoolean("ready");
+ assert obj.getBoolean("isLeader");
+ assert "STATE_LEADER".equals(obj.getString("state"));
+ // Unauthenticated, so it must not disclose cluster addresses
+ assert !obj.has("leader") : "the anonymous body must not carry the
leader address";
+ }
+
+ @Test
+ public void testRaftGaugesExported() throws URISyntaxException,
IOException,
+ InterruptedException {
+ String url = pdRestAddr + "/actuator/prometheus";
+ HttpRequest request = HttpRequest.newBuilder().uri(new
URI(url)).GET().build();
+ HttpResponse<String> response = client.send(request,
HttpResponse.BodyHandlers.ofString());
+ assert response.statusCode() == 200;
+ String body = response.body();
+ assert body.contains("hg_raft_leader{") : "missing hg_raft_leader
gauge";
+ assert body.contains("hg_raft_has_leader{") : "missing
hg_raft_has_leader gauge";
+ assert body.contains("hg_raft_alive_peers{") : "missing
hg_raft_alive_peers gauge";
+ // Single-node CI cluster: this PD is the leader and hears from itself
+ assert body.matches("(?s).*hg_raft_leader\\{[^}]*\\} 1\\.0.*") :
+ "hg_raft_leader should be 1 on a single-node leader";
+ assert body.matches("(?s).*hg_raft_has_leader\\{[^}]*\\} 1\\.0.*") :
+ "hg_raft_has_leader should be 1 on a single-node leader";
+ assert body.matches("(?s).*hg_raft_alive_peers\\{[^}]*\\} 1\\.0.*") :
+ "hg_raft_alive_peers should be 1 on a single-node leader";
Review Comment:
Fixed in `cf95f22ee`. The label group is optional now.
One thing that came out of doing it: making the braces optional on its own
let the `# TYPE hg_raft_leader gauge` line satisfy the presence check, so each
pattern is also anchored to the start of a line, which only a sample line
matches. Checked all six against tagged, untagged, zero-valued, NaN and
HELP/TYPE-only bodies, including that `hg_raft_leader` does not match an
`hg_raft_has_leader` line.
For the record this is hardening rather than a live failure: `MetricsConfig`
adds a common `hg="pd"` tag, so every sample line carries a label block in this
build.
--
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]
