bitflicker64 commented on code in PR #3185:
URL: https://github.com/apache/hugegraph/pull/3185#discussion_r3934249051
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java:
##########
@@ -203,7 +204,101 @@ 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();
+ }
+
+ /**
+ * Take a consistent view of the local raft state. Every field is derived
from one
+ * {@link Node} reference and a single {@code getLeaderId()} read, so a
step-down while
+ * the view is being built cannot report a ready node that knows no leader.
+ * <p>
+ * A node is ready when it has been started, is in an active state, which
jraft's
+ * {@code State.isActive()} takes to mean leader, transferring, candidate
or follower,
+ * and sees a leader. Unlike a plain liveness check this turns false as
soon as the
+ * quorum is lost.
+ */
+ public RaftStatus getRaftStatus() {
+ Node node = this.raftNode;
+ if (node == null) {
+ return new RaftStatus(false, State.STATE_UNINITIALIZED.name(),
false);
+ }
+ State state = node.getNodeState();
+ boolean active = state != null && state.isActive();
+ return new RaftStatus(active && hasLeader(node),
+ state == null ? State.STATE_UNINITIALIZED.name()
: state.name(),
+ node.isLeader(true));
Review Comment:
๐งน The javadoc above says "Every field is derived from one `Node` reference
and a single `getLeaderId()` read, so a step-down while the view is being built
cannot report a ready node that knows no leader." The `ready` flag does hold
that property, but the snapshot as a whole does not: this method takes three
independent reads of the node, each acquiring `NodeImpl`'s read lock separately
โ `getNodeState()` on L246, `getLeaderId()` via `hasLeader(node)`, and
`isLeader(true)` here. Constructor arguments evaluate left to right, so a
step-down between the `hasLeader` and `isLeader` reads emits a
self-contradictory body such as
`{"ready":true,"state":"STATE_LEADER","isLeader":false}`.
Harmless for a probe that only reads `ready`, but the javadoc promises a
guarantee the code does not provide, and a later reader may lean on it. Since
`state` has already been read one line up, `localLeader` can just be derived
from it โ that makes all three fields genuinely consistent and drops a
redundant read-lock acquisition on every `/v1/ready` request and every metrics
scrape:
```suggestion
State.STATE_LEADER == state);
```
##########
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)
Review Comment:
๐งน `hg.raft.has_leader` (and `hg.raft.alive_peers` on L95) embed an
underscore inside a dot-delimited name segment. Micrometer's convention is
lowercase dot-separated words, with each registry's `NamingConvention` doing
the separator translation โ that is why `hg.raft.leader` on L89 needs nothing.
The Prometheus output is the intended `hg_raft_has_leader` either way, but on
any other registry the underscore renders literally and yields a
mixed-separator name.
`hg.raft.has.leader` and `hg.raft.alive.peers` produce byte-identical
Prometheus output (so the `RestApiTest` assertions and the docs table are
unaffected) while staying portable:
```suggestion
Gauge.builder(PREFIX + ".raft.has.leader", () -> raft.hasLeader() ?
1 : 0)
```
##########
hugegraph-server/hugegraph-dist/src/assembly/travis/test-start-hugegraph-pd.sh:
##########
@@ -111,6 +111,19 @@ wait_for_pd() {
return 1
}
+# Wait until PD readiness answers, or timeout. This is the gate the docs
recommend:
+# -f rejects the 503, the body match rejects a 200 that is an auth envelope
rather
+# than a readiness answer. No credentials, and a single-node group elects
itself.
+wait_for_pd_ready() {
+ local elapsed=0
+ while (( elapsed < STARTUP_WAIT )); do
+ curl -fsS "$PD_URL/v1/ready" 2>/dev/null | grep -q '"ready":true' &&
return 0
Review Comment:
๐งน This script runs under `set -uo pipefail` (L28), and this line is the only
pipeline in it. `grep -q` exits the moment it matches, so `curl` can be killed
by `SIGPIPE`; with `pipefail` the pipeline status then becomes non-zero, `&&
return 0` never fires, and the loop polls to the full `STARTUP_WAIT` before
`fail`ing โ on a PD that was in fact ready. With a ~55-byte body curl almost
always completes its single write before grep exits, so this is unlikely to
bite in practice, but it makes a CI gate depend on a race.
The adjacent `wait_for_pd()` (L99-112) already avoids exactly this by
capturing into a variable instead of piping. Same shape here removes the
dependency and keeps the two functions stylistically consistent:
```suggestion
local body
body=$(curl -fsS "$PD_URL/v1/ready" 2>/dev/null || true)
grep -q '"ready":true' <<<"$body" && return 0
```
--
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]
