From fc017f801c2309ccc3f21704e7eb322782ca6121 Mon Sep 17 00:00:00 2001 From: Himanshu Verma Date: Wed, 23 Sep 2026 19:02:20 +0530 Subject: [PATCH 1/2] fix(store): keep GET /v1/partitions working on follower stores PartitionAPI.getPartitions built each raft group's conf through PartitionEngine.getCurrentConf(), which calls jraft Node.listPeers(). jraft only allows that on the leader, so the endpoint failed with a 500 "Not leader" at the first group the store follows. Every store follows some groups once replicas > 1, so the endpoint failed cluster-wide. Move the conf into the existing isLeader() block that already guards peers and learners. Followers now report conf as null, matching peers and learners. Also add TODO comments at dead code found nearby, with no behavior change: a dead return and an unused DashResponse line in PartitionAPI, a commented-out changeShards block, an unused groupId with an unlogged error in doBlankTaskSync and a silent replicator onError in PartitionEngine, and HgStoreNodeServiceTest, whose tests are all commented out. Fixes #3230 --- .../hugegraph/store/PartitionEngine.java | 3 + .../store/node/controller/PartitionAPI.java | 5 +- .../store/node/HgStoreNodeServiceTest.java | 1 + .../node/controller/PartitionAPITest.java | 112 ++++++++++++++++++ 4 files changed, 120 insertions(+), 1 deletion(-) create mode 100644 hugegraph-store/hg-store-node/src/test/java/org/apache/hugegraph/store/node/controller/PartitionAPITest.java diff --git a/hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/PartitionEngine.java b/hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/PartitionEngine.java index a70f17465f..1b6c8d06bf 100644 --- a/hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/PartitionEngine.java +++ b/hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/PartitionEngine.java @@ -659,6 +659,7 @@ public void onConfigurationCommitted(Configuration conf) { partitionManager.updateShardGroup(shardGroup); if (isLeader()) { + // TODO: remove this commented-out block if nothing still needs it // partitionManager.getPartitionList(getGroupId()).forEach(partition -> { // partitionManager.changeShards(partition, shardGroup.getMetaPbShard()); // }); @@ -1133,6 +1134,7 @@ public void doBlankTaskSync(Closure done) { try { doSnapshotSync(done); } catch (Exception e) { + // TODO: groupId is unused and the error is never logged; log it or remove both Integer groupId = getGroupId(); // String msg = String.format("Partition %s blank task done with error：", groupId); // log.error(msg, e); @@ -1159,6 +1161,7 @@ public void onCreated(PeerId peer) { @Override public void onError(PeerId peer, Status status) { + // TODO: replicator errors are silently dropped; log them or remove this line // log.info("Raft {} Replicator onError {} {}", getGroupId(), peer, status); } diff --git a/hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java b/hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java index 34f03642ed..ec0f1490eb 100644 --- a/hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java +++ b/hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java @@ -87,8 +87,9 @@ public Map 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()); raft.setPeers(engine.getRaftNode().listPeers()); raft.setLearners(engine.getRaftNode().listLearners()); } @@ -139,6 +140,7 @@ public Raft getPartition(@PathVariable(value = "id") int id) { } return raft; + // TODO: remove this dead return, the method already returns raft above //return okMap("partition", rafts); } @@ -195,6 +197,7 @@ public Map arthasstart( configMap.put("arthas.ip", appConfig.getArthasConfig().getArthasip()); configMap.put("arthas.disabledCommands", appConfig.getArthasConfig().getDisCmd()); ArthasAgent.attach(configMap); + // TODO: remove this commented-out line, retPose is never used // DashResponse retPose = new DashResponse(); List ret = new ArrayList<>(); ret.add("Arthas started successfully"); diff --git a/hugegraph-store/hg-store-node/src/test/java/org/apache/hugegraph/store/node/HgStoreNodeServiceTest.java b/hugegraph-store/hg-store-node/src/test/java/org/apache/hugegraph/store/node/HgStoreNodeServiceTest.java index 336428f998..99399e0aa9 100644 --- a/hugegraph-store/hg-store-node/src/test/java/org/apache/hugegraph/store/node/HgStoreNodeServiceTest.java +++ b/hugegraph-store/hg-store-node/src/test/java/org/apache/hugegraph/store/node/HgStoreNodeServiceTest.java @@ -33,6 +33,7 @@ * 3, test copy addition and subtraction * 4. Test single frame with log storage turned off */ +// TODO: every @Test here is commented out, so this class runs nothing; write real tests or remove it public class HgStoreNodeServiceTest { String yml = diff --git a/hugegraph-store/hg-store-node/src/test/java/org/apache/hugegraph/store/node/controller/PartitionAPITest.java b/hugegraph-store/hg-store-node/src/test/java/org/apache/hugegraph/store/node/controller/PartitionAPITest.java new file mode 100644 index 0000000000..8c263495dc --- /dev/null +++ b/hugegraph-store/hg-store-node/src/test/java/org/apache/hugegraph/store/node/controller/PartitionAPITest.java @@ -0,0 +1,112 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hugegraph.store.node.controller; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import org.apache.hugegraph.store.HgStoreEngine; +import org.apache.hugegraph.store.PartitionEngine; +import org.apache.hugegraph.store.business.BusinessHandler; +import org.apache.hugegraph.store.node.grpc.HgStoreNodeService; +import org.junit.Assert; +import org.junit.Test; + +import com.alipay.sofa.jraft.Node; +import com.alipay.sofa.jraft.conf.Configuration; +import com.alipay.sofa.jraft.core.State; +import com.alipay.sofa.jraft.entity.PeerId; + +public class PartitionAPITest { + + private static final String NOT_LEADER = "Not leader"; + + @Test + public void testGetPartitionsOnFollower() { + PartitionEngine follower = mockEngine(1, false); + PartitionEngine leader = mockEngine(2, true); + PartitionAPI api = newApi(follower, leader); + + @SuppressWarnings("unchecked") + List rafts = + (List) api.getPartitions("").get("partitions"); + + Assert.assertEquals(2, rafts.size()); + PartitionAPI.Raft followerRaft = rafts.get(0); + Assert.assertEquals(1, followerRaft.getGroupId()); + Assert.assertEquals(State.STATE_FOLLOWER.name(), followerRaft.getRole()); + Assert.assertNull(followerRaft.getConf()); + Assert.assertNull(followerRaft.getPeers()); + Assert.assertNull(followerRaft.getLearners()); + + PartitionAPI.Raft leaderRaft = rafts.get(1); + Assert.assertEquals(2, leaderRaft.getGroupId()); + Assert.assertEquals(State.STATE_LEADER.name(), leaderRaft.getRole()); + Assert.assertEquals("127.0.0.1:8510", leaderRaft.getConf()); + Assert.assertEquals(Collections.singletonList(PeerId.parsePeer("127.0.0.1:8510")), + leaderRaft.getPeers()); + } + + private static PartitionAPI newApi(PartitionEngine... engines) { + Map partitionEngines = new LinkedHashMap<>(); + for (PartitionEngine engine : engines) { + partitionEngines.put(engine.getGroupId(), engine); + } + HgStoreEngine storeEngine = mock(HgStoreEngine.class); + when(storeEngine.getBusinessHandler()).thenReturn(mock(BusinessHandler.class)); + when(storeEngine.getPartitionEngines()).thenReturn(partitionEngines); + HgStoreNodeService nodeService = mock(HgStoreNodeService.class); + when(nodeService.getStoreEngine()).thenReturn(storeEngine); + + PartitionAPI api = new PartitionAPI(); + api.nodeService = nodeService; + return api; + } + + private static PartitionEngine mockEngine(int groupId, boolean isLeader) { + // Same contract as jraft NodeImpl: peer and learner lists are leader-only + Node node = mock(Node.class); + List peers = Collections.singletonList(PeerId.parsePeer("127.0.0.1:8510")); + if (isLeader) { + when(node.getNodeState()).thenReturn(State.STATE_LEADER); + when(node.listPeers()).thenReturn(peers); + when(node.listLearners()).thenReturn(Collections.emptyList()); + } else { + when(node.getNodeState()).thenReturn(State.STATE_FOLLOWER); + when(node.listPeers()).thenThrow(new IllegalStateException(NOT_LEADER)); + when(node.listLearners()).thenThrow(new IllegalStateException(NOT_LEADER)); + } + + PartitionEngine engine = mock(PartitionEngine.class); + when(engine.getGroupId()).thenReturn(groupId); + when(engine.isLeader()).thenReturn(isLeader); + when(engine.getRaftNode()).thenReturn(node); + when(engine.getPartitions()).thenReturn(Collections.emptyMap()); + if (isLeader) { + when(engine.getCurrentConf()).thenReturn(new Configuration(peers)); + } else { + when(engine.getCurrentConf()).thenThrow(new IllegalStateException(NOT_LEADER)); + } + return engine; + } +} From 8dd36653aeaedc3543c88eea7a4bba6c6e90a535 Mon Sep 17 00:00:00 2001 From: Himanshu Verma Date: Thu, 24 Sep 2026 12:48:48 +0530 Subject: [PATCH 2/2] fix(store): tolerate a leader step-down while listing partitions engine.isLeader() calls jraft isLeader(false), which reads the node state without the node lock. getCurrentConf(), listPeers() and listLearners() then take the lock and throw "Not leader" if the group stepped down in between, for example during a PD leader balance, which still turned GET /v1/partitions into a 500. Read the three values inside one try block and set them only if all succeed. On IllegalStateException, log it and leave conf, peers and learners null, the same as for a follower. --- .../store/node/controller/PartitionAPI.java | 17 +++++++++--- .../node/controller/PartitionAPITest.java | 26 +++++++++++++++++-- 2 files changed, 37 insertions(+), 6 deletions(-) diff --git a/hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java b/hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java index ec0f1490eb..39f500ad93 100644 --- a/hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java +++ b/hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java @@ -87,11 +87,20 @@ public Map getPartitions( raft.setGroupId(engine.getGroupId()); raft.setLeader(engine.getLeader()); raft.setRole(engine.getRaftNode().getNodeState().name()); - // jraft only lists peers and learners on the leader + // jraft only lists peers and learners on the leader, and isLeader() does not + // hold the node lock, so leadership can still move before these calls if (engine.isLeader()) { - raft.setConf(engine.getCurrentConf().toString()); - raft.setPeers(engine.getRaftNode().listPeers()); - raft.setLearners(engine.getRaftNode().listLearners()); + try { + String conf = engine.getCurrentConf().toString(); + List peers = engine.getRaftNode().listPeers(); + List learners = engine.getRaftNode().listLearners(); + raft.setConf(conf); + raft.setPeers(peers); + raft.setLearners(learners); + } catch (IllegalStateException e) { + log.info("Raft {} is no longer leader, skip its conf: {}", + engine.getGroupId(), e.getMessage()); + } } raft.setTerm(engine.getLeaderTerm()); raft.setLogIndex(engine.getCommittedIndex()); diff --git a/hugegraph-store/hg-store-node/src/test/java/org/apache/hugegraph/store/node/controller/PartitionAPITest.java b/hugegraph-store/hg-store-node/src/test/java/org/apache/hugegraph/store/node/controller/PartitionAPITest.java index 8c263495dc..b37565c8ab 100644 --- a/hugegraph-store/hg-store-node/src/test/java/org/apache/hugegraph/store/node/controller/PartitionAPITest.java +++ b/hugegraph-store/hg-store-node/src/test/java/org/apache/hugegraph/store/node/controller/PartitionAPITest.java @@ -67,6 +67,24 @@ public void testGetPartitionsOnFollower() { leaderRaft.getPeers()); } + @Test + public void testGetPartitionsWhenLeaderStepsDown() { + // isLeader() still returns true, but the node stepped down before listPeers() + PartitionEngine steppedDown = mockEngine(1, true, false); + PartitionAPI api = newApi(steppedDown); + + @SuppressWarnings("unchecked") + List rafts = + (List) api.getPartitions("").get("partitions"); + + Assert.assertEquals(1, rafts.size()); + PartitionAPI.Raft raft = rafts.get(0); + Assert.assertEquals(1, raft.getGroupId()); + Assert.assertNull(raft.getConf()); + Assert.assertNull(raft.getPeers()); + Assert.assertNull(raft.getLearners()); + } + private static PartitionAPI newApi(PartitionEngine... engines) { Map partitionEngines = new LinkedHashMap<>(); for (PartitionEngine engine : engines) { @@ -84,10 +102,14 @@ private static PartitionAPI newApi(PartitionEngine... engines) { } private static PartitionEngine mockEngine(int groupId, boolean isLeader) { + return mockEngine(groupId, isLeader, isLeader); + } + + private static PartitionEngine mockEngine(int groupId, boolean isLeader, boolean nodeLeads) { // Same contract as jraft NodeImpl: peer and learner lists are leader-only Node node = mock(Node.class); List peers = Collections.singletonList(PeerId.parsePeer("127.0.0.1:8510")); - if (isLeader) { + if (nodeLeads) { when(node.getNodeState()).thenReturn(State.STATE_LEADER); when(node.listPeers()).thenReturn(peers); when(node.listLearners()).thenReturn(Collections.emptyList()); @@ -102,7 +124,7 @@ private static PartitionEngine mockEngine(int groupId, boolean isLeader) { when(engine.isLeader()).thenReturn(isLeader); when(engine.getRaftNode()).thenReturn(node); when(engine.getPartitions()).thenReturn(Collections.emptyMap()); - if (isLeader) { + if (nodeLeads) { when(engine.getCurrentConf()).thenReturn(new Configuration(peers)); } else { when(engine.getCurrentConf()).thenThrow(new IllegalStateException(NOT_LEADER));