bitflicker64 commented on code in PR #3185:
URL: https://github.com/apache/hugegraph/pull/3185#discussion_r3954360894
##########
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:
๐งน `hasLeader()` omits the `state.isActive()` guard that `getRaftStatus()`
applies at L264 (`view.state.isActive() && view.seesLeader`), yet
`PDMetrics.registerRaftMeters()` documents the gauges as mirroring what `GET
/v1/ready` answers โ and `hg_raft_has_leader` is fed by this method while
`/v1/ready` is fed by `getRaftStatus()`.
The two agree today only by accident of which callbacks write what: the two
that set `seesLeader = true` (`onLeaderStart`, `onStartFollowing`) happen to
pair it with an active state, and the two that force an inactive state
(`onError`, `onShutdown`) happen to clear it. `ProbeView` does not enforce that
pairing, so a future callback writing an inactive state without clearing
`seesLeader` would silently split the gauge from the endpoint it is documented
to mirror.
Requested change: derive this from the same predicate the endpoint uses,
e.g. `return getRaftStatus().isReady();`, or say in the javadoc why the
active-state guard is deliberately omitted here.
##########
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;
+ }
+
+ /**
+ * 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() {
Review Comment:
๐งน The PR description no longer describes this head, and since the repository
squash-merges, the body becomes the commit message.
"Main Changes" says `RaftEngine` gains "`hasLeader()`, `isReady()`,
`getNodeState()` and `getAlivePeerCount()`". At `63a2690` the new public
surface is `hasLeader()`, `getRaftStatus()` and `getAlivePeerCount()` plus the
nested `RaftStatus`; there is no `RaftEngine.isReady()` and no `getNodeState()`
(`isReady()` lives only on `RaftStatus`, L290).
"Verify the Changes" lists `RaftEngineReadinessTest` cases for "empty leader
id, candidate, transferring" that the file does not contain โ its ten tests are
`testNotReadyBeforeRaftNodeStarts`,
`testStartedNodeWithoutAnyCallbackIsNotReady`, `testLeaderIsReady`,
`testFollowerWithLeaderIsReady`, `testFollowerLosingItsLeaderTurnsNotReady`,
`testLeaderSteppingDownTurnsNotReady`,
`testErrorAndShutdownAreNotReadyEvenAfterLeadership`,
`testProbeNeverTouchesTheRaftNode`,
`testAlivePeerCountSurvivesLeadershipLossRace` and
`testAlivePeerCountOnALeaderAlsoSkipsTheNode` โ and it never mentions the new
`StoreAPIReadyTest`.
Requested change: refresh both sections against `63a2690` before merge โ
name `getRaftStatus()` and the `RaftStatus` view, drop
`isReady()`/`getNodeState()`, correct the test-case list, and add
`StoreAPIReadyTest`.
##########
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:
๐งน This wait cannot deliver the guarantee the comment above it states. The
case it names is a refresh already blocked inside `NodeImpl.listAlivePeers()`,
which parks on `ReentrantReadWriteLock.readLock().lock()` โ not interruptible,
so `shutdownNow()`'s interrupt does not release it. The hold time this whole
design exists to avoid is bounded by the raft RPC connect timeout, which
`init()` sets from `config.getRpcTimeout()` (`PDConfig.java:153`, default
`raft.rpc-timeout: 10000`), an order of magnitude past the 1 s wait. The
`false` return is discarded too, so `shutDown()` proceeds to
`this.alivePeerCount = -1` and the late refresh can still publish a positive
count on top of it โ exactly the outcome the comment says is prevented.
Impact is small today: `shutDown()` has no production caller (only
`PDCoreTestBase:187` and `PdTestBase:189`), and
`RaftEngineReadinessTest.setUp()` re-runs `refreshAlivePeerCount()` to clear
whatever an earlier class published. But the comment asserts a property the
code does not have, and it becomes a real stale-gauge path as soon as
`shutDown()` is wired into a lifecycle hook.
Requested change: make the publish conditional rather than relying on the
wait โ have `refreshAlivePeerCount()` write only while a `running` flag (or a
generation token captured at schedule time) is still current โ or at minimum
check the return value, reset `alivePeerCount` after it, and log when the
refresher did not stop in time.
--
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]
