bitflicker64 commented on code in PR #3185:
URL: https://github.com/apache/hugegraph/pull/3185#discussion_r3956277746
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java:
##########
@@ -203,7 +230,130 @@ 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;
+ }
Review Comment:
Applyable form of the earlier `hasLeader()` comment on this method: derive
the gauge from the predicate `/v1/ready` answers with, so `hg_raft_has_leader`
cannot drift from the endpoint it is documented to mirror if a future callback
pairs an inactive state with `seesLeader`. Behaviour today is identical: a null
`raftNode` still answers false, and every callback that sets `seesLeader` pairs
it with an active state. Traced through all ten `RaftEngineReadinessTest`
cases; the no-node-lock verification still holds because reading the `raftNode`
field is not a mock interaction. If a one-line alias next to its only caller
reads as too thin, inlining `getRaftStatus().isReady()` at the `PDMetrics` call
site works just as well.
```suggestion
/**
* Whether this PD is ready in the sense {@code GET /v1/ready} answers:
the raft node
* is active and sees a 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 getRaftStatus().isReady();
}
```
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java:
##########
@@ -182,6 +196,19 @@ public List<ChannelHandler> backChannelHandlers() {
}
public void shutDown() {
+ if (this.alivePeersRefresher != null) {
+ this.alivePeersRefresher.shutdownNow();
+ try {
+ // shutdownNow only interrupts; a refresh already inside
+ // listAlivePeers could otherwise publish a positive count
+ // after the reset below.
+ this.alivePeersRefresher.awaitTermination(1, TimeUnit.SECONDS);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
Review Comment:
Applyable form of the earlier `shutDown()` comment, the minimal version:
keep the wait, stop claiming it prevents the late publish (the blocked refresh
sits in a lock acquire that ignores the interrupt, for up to PD's
`raft.rpc-timeout`, default 10 s, per unreachable peer), and log when the
refresher outlives the wait. The stronger fix, publishing only while a
generation token captured at schedule time is current, needs a second field and
a change in `refreshAlivePeerCount()`; worth doing once `shutDown()` gains a
production caller, which today it does not have.
```suggestion
try {
// Best effort: shutdownNow only interrupts, and a refresh
parked in
// listAlivePeers waits on a lock acquire the interrupt does
not break,
// for up to the raft rpc connect timeout per unreachable
peer. A refresh
// that outlives this wait may publish one stale count over
the reset
// below; acceptable while shutDown has no production caller.
if (!this.alivePeersRefresher.awaitTermination(1,
TimeUnit.SECONDS)) {
log.warn("Raft alive-peers refresher still running after
shutdown; " +
"hg_raft_alive_peers may briefly report a stale
value");
}
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
```
--
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]
