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..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,10 +87,20 @@ 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, and isLeader() does not + // hold the node lock, so leadership can still move before these calls if (engine.isLeader()) { - 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()); @@ -139,6 +149,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 +206,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..b37565c8ab --- /dev/null +++ b/hugegraph-store/hg-store-node/src/test/java/org/apache/hugegraph/store/node/controller/PartitionAPITest.java @@ -0,0 +1,134 @@ +/* + * 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()); + } + + @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) { + 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) { + 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 (nodeLeads) { + 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 (nodeLeads) { + when(engine.getCurrentConf()).thenReturn(new Configuration(peers)); + } else { + when(engine.getCurrentConf()).thenThrow(new IllegalStateException(NOT_LEADER)); + } + return engine; + } +}