bitflicker64 commented on code in PR #3185:
URL: https://github.com/apache/hugegraph/pull/3185#discussion_r3957144743
##########
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() {
Review Comment:
Fixed in 667b6fb. `hasLeader()` is now `getRaftStatus().isReady()`, so
`hg_raft_has_leader` and `GET /v1/ready` answer the same predicate and cannot
split if a future callback pairs an inactive state with `seesLeader`. Behaviour
is unchanged today. Javadoc updated to say the method answers readiness rather
than bare leader visibility.
Verified at `eb6de8f` on JDK 11: `mvn test -pl hugegraph-pd/hg-pd-test -am
-P pd-core-test` passes 104/104 (2 skipped), including all twelve
`RaftEngineReadinessTest` cases. `testProbeNeverTouchesTheRaftNode` still
holds: the method no longer reads the `raftNode` field at all.
##########
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);
Review Comment:
Fixed in 667b6fb, the minimal version from the suggestion. The wait stays,
the comment now says what it actually buys (a refresh parked in
`listAlivePeers()` sits on a lock acquire the interrupt does not break, for up
to the raft rpc connect timeout per unreachable peer), and the return value is
checked so an overrun logs a warning naming the gauge that may go stale.
Took the minimal fix over the generation token deliberately: the token needs
a second field and a change in `refreshAlivePeerCount()`, whose signature the
core tests call directly, and `shutDown()` still has no production caller.
Worth revisiting when it gains one.
Verified at `eb6de8f`: `pd-core-test` 104/104 (2 skipped).
--
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]
