bitflicker64 commented on code in PR #3232:
URL: https://github.com/apache/hugegraph/pull/3232#discussion_r4085977816
##########
hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java:
##########
@@ -87,8 +87,9 @@ public Map<String, Object> getPartitions(
raft.setGroupId(engine.getGroupId());
raft.setLeader(engine.getLeader());
raft.setRole(engine.getRaftNode().getNodeState().name());
- raft.setConf(engine.getCurrentConf().toString());
+ // jraft only lists peers and learners on the leader
if (engine.isLeader()) {
+ raft.setConf(engine.getCurrentConf().toString());
Review Comment:
Minor: `engine.isLeader()` calls `raftNode.isLeader(false)`, which reads
`state` without the node lock. `getCurrentConf()`, `listPeers()` and
`listLearners()` then each take the read lock and throw
`IllegalStateException("Not leader")` if `state` is no longer `STATE_LEADER`
(jraft-core 1.3.13 `NodeImpl.listPeers`). If the group steps down between the
check and these calls, for example during a PD leader balance, the whole `GET
/v1/partitions` response is still a 500. This race existed before for
`listPeers()` and the PR does not make it worse, so it does not block.
If you want the endpoint to survive leader transfers too, wrap the three
calls in one `try { ... } catch (IllegalStateException e) { ... }` and leave
`conf`, `peers` and `learners` null on failure, the same as the follower case.
A second follower-style mock whose `isLeader()` returns true while the node
methods throw would cover it.
--
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]
