Skip to content

Commit 6d5cbab

Browse files
CRZbulabulaJackieTien97
authored andcommitted
Fix AddConfigNode retry idempotency (#17874)
1 parent 34677ab commit 6d5cbab

4 files changed

Lines changed: 192 additions & 3 deletions

File tree

iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/consensus/ConsensusManager.java

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import org.apache.iotdb.commons.conf.CommonDescriptor;
2828
import org.apache.iotdb.commons.consensus.ConfigRegionId;
2929
import org.apache.iotdb.commons.consensus.ConsensusGroupId;
30+
import org.apache.iotdb.commons.utils.TestOnly;
3031
import org.apache.iotdb.confignode.conf.ConfigNodeConfig;
3132
import org.apache.iotdb.confignode.conf.ConfigNodeDescriptor;
3233
import org.apache.iotdb.confignode.conf.SystemPropertiesUtils;
@@ -44,6 +45,9 @@
4445
import org.apache.iotdb.consensus.config.ConsensusConfig;
4546
import org.apache.iotdb.consensus.config.RatisConfig;
4647
import org.apache.iotdb.consensus.exception.ConsensusException;
48+
import org.apache.iotdb.consensus.exception.ConsensusGroupAlreadyExistException;
49+
import org.apache.iotdb.consensus.exception.PeerAlreadyInConsensusGroupException;
50+
import org.apache.iotdb.consensus.exception.PeerNotInConsensusGroupException;
4751
import org.apache.iotdb.db.protocol.client.ConfigNodeInfo;
4852
import org.apache.iotdb.rpc.TSStatusCode;
4953

@@ -86,6 +90,12 @@ public ConsensusManager(IManager configManager, ConfigRegionStateMachine stateMa
8690
setConsensusLayer(stateMachine);
8791
}
8892

93+
@TestOnly
94+
ConsensusManager(IManager configManager, IConsensus consensusImpl) {
95+
this.configManager = configManager;
96+
this.consensusImpl = consensusImpl;
97+
}
98+
8999
public void start() throws IOException {
90100
consensusImpl.start();
91101
if (SystemPropertiesUtils.isRestarted()) {
@@ -289,7 +299,11 @@ public void createPeerForConsensusGroup(List<TConfigNodeLocation> configNodeLoca
289299
configNodeLocation.getConfigNodeId(),
290300
configNodeLocation.getConsensusEndPoint()));
291301
}
292-
consensusImpl.createLocalPeer(DEFAULT_CONSENSUS_GROUP_ID, peerList);
302+
try {
303+
consensusImpl.createLocalPeer(DEFAULT_CONSENSUS_GROUP_ID, peerList);
304+
} catch (ConsensusGroupAlreadyExistException e) {
305+
LOGGER.info("ConfigNode local peer has already been created: {}", e.getMessage());
306+
}
293307
}
294308

295309
/**
@@ -306,6 +320,9 @@ public void addConfigNodePeer(TConfigNodeLocation configNodeLocation) throws Add
306320
DEFAULT_CONSENSUS_GROUP_ID,
307321
configNodeLocation.getConfigNodeId(),
308322
configNodeLocation.getConsensusEndPoint()));
323+
} catch (PeerAlreadyInConsensusGroupException e) {
324+
LOGGER.info(
325+
"ConfigNode peer {} has already been added: {}", configNodeLocation, e.getMessage());
309326
} catch (ConsensusException e) {
310327
throw new AddPeerException(configNodeLocation);
311328
}
@@ -327,6 +344,10 @@ public boolean removeConfigNodePeer(TConfigNodeLocation configNodeLocation) {
327344
configNodeLocation.getConfigNodeId(),
328345
configNodeLocation.getConsensusEndPoint()));
329346
return true;
347+
} catch (PeerNotInConsensusGroupException e) {
348+
LOGGER.info(
349+
"ConfigNode peer {} has already been removed: {}", configNodeLocation, e.getMessage());
350+
return true;
330351
} catch (ConsensusException e) {
331352
return false;
332353
}

iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/service/thrift/ConfigNodeRPCServiceProcessor.java

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,7 @@
230230
import org.apache.iotdb.confignode.rpc.thrift.TUnsubscribeReq;
231231
import org.apache.iotdb.confignode.service.ConfigNode;
232232
import org.apache.iotdb.consensus.exception.ConsensusException;
233+
import org.apache.iotdb.consensus.exception.ConsensusGroupNotExistException;
233234
import org.apache.iotdb.db.queryengine.plan.relational.type.AuthorRType;
234235
import org.apache.iotdb.rpc.RpcUtils;
235236
import org.apache.iotdb.rpc.TSStatusCode;
@@ -859,15 +860,19 @@ public TSStatus removeConfigNode(TConfigNodeLocation configNodeLocation) throws
859860

860861
@Override
861862
public TSStatus deleteConfigNodePeer(TConfigNodeLocation configNodeLocation) {
862-
if (!configManager.getNodeManager().getRegisteredConfigNodes().contains(configNodeLocation)) {
863+
if (configNodeConfig.getConfigNodeId() != -1
864+
&& configNodeLocation.getConfigNodeId() != configNodeConfig.getConfigNodeId()) {
863865
return new TSStatus(TSStatusCode.REMOVE_CONFIGNODE_ERROR.getStatusCode())
864866
.setMessage(
865-
"remove ConsensusGroup failed because the ConfigNode not in current Cluster.");
867+
"remove ConsensusGroup failed because the target ConfigNode is not current ConfigNode.");
866868
}
867869

868870
ConsensusGroupId groupId = configManager.getConsensusManager().getConsensusGroupId();
869871
try {
870872
configManager.getConsensusManager().getConsensusImpl().deleteLocalPeer(groupId);
873+
} catch (ConsensusGroupNotExistException e) {
874+
return new TSStatus(TSStatusCode.SUCCESS_STATUS.getStatusCode())
875+
.setMessage(ConfigNodeMessages.REMOVE_CONSENSUSGROUP_SUCCESS);
871876
} catch (ConsensusException e) {
872877
return new TSStatus(TSStatusCode.REMOVE_CONFIGNODE_ERROR.getStatusCode())
873878
.setMessage(
Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.apache.iotdb.confignode.manager.consensus;
21+
22+
import org.apache.iotdb.common.rpc.thrift.TConfigNodeLocation;
23+
import org.apache.iotdb.common.rpc.thrift.TEndPoint;
24+
import org.apache.iotdb.confignode.exception.AddPeerException;
25+
import org.apache.iotdb.confignode.manager.IManager;
26+
import org.apache.iotdb.consensus.IConsensus;
27+
import org.apache.iotdb.consensus.common.Peer;
28+
import org.apache.iotdb.consensus.exception.ConsensusException;
29+
import org.apache.iotdb.consensus.exception.ConsensusGroupAlreadyExistException;
30+
import org.apache.iotdb.consensus.exception.PeerAlreadyInConsensusGroupException;
31+
import org.apache.iotdb.consensus.exception.PeerNotInConsensusGroupException;
32+
33+
import org.junit.Assert;
34+
import org.junit.Test;
35+
import org.mockito.Mockito;
36+
37+
import java.util.Collections;
38+
39+
public class ConsensusManagerTest {
40+
41+
@Test
42+
public void createPeerForConsensusGroupShouldIgnoreAlreadyCreatedLocalPeer() throws Exception {
43+
final IConsensus consensus = Mockito.mock(IConsensus.class);
44+
Mockito.doThrow(
45+
new ConsensusGroupAlreadyExistException(ConsensusManager.DEFAULT_CONSENSUS_GROUP_ID))
46+
.when(consensus)
47+
.createLocalPeer(
48+
Mockito.eq(ConsensusManager.DEFAULT_CONSENSUS_GROUP_ID), Mockito.anyList());
49+
50+
newConsensusManager(consensus)
51+
.createPeerForConsensusGroup(Collections.singletonList(newConfigNodeLocation(1)));
52+
53+
Mockito.verify(consensus)
54+
.createLocalPeer(
55+
Mockito.eq(ConsensusManager.DEFAULT_CONSENSUS_GROUP_ID), Mockito.anyList());
56+
}
57+
58+
@Test
59+
public void addConfigNodePeerShouldIgnoreAlreadyAddedPeer() throws Exception {
60+
final IConsensus consensus = Mockito.mock(IConsensus.class);
61+
Mockito.doThrow(
62+
new PeerAlreadyInConsensusGroupException(
63+
ConsensusManager.DEFAULT_CONSENSUS_GROUP_ID,
64+
new Peer(
65+
ConsensusManager.DEFAULT_CONSENSUS_GROUP_ID,
66+
1,
67+
new TEndPoint("127.0.0.1", 10720))))
68+
.when(consensus)
69+
.addRemotePeer(
70+
Mockito.eq(ConsensusManager.DEFAULT_CONSENSUS_GROUP_ID), Mockito.any(Peer.class));
71+
72+
newConsensusManager(consensus).addConfigNodePeer(newConfigNodeLocation(1));
73+
74+
Mockito.verify(consensus)
75+
.addRemotePeer(
76+
Mockito.eq(ConsensusManager.DEFAULT_CONSENSUS_GROUP_ID), Mockito.any(Peer.class));
77+
}
78+
79+
@Test
80+
public void addConfigNodePeerShouldKeepFailingForOtherConsensusErrors() throws Exception {
81+
final IConsensus consensus = Mockito.mock(IConsensus.class);
82+
Mockito.doThrow(new ConsensusException("reconfiguration failed"))
83+
.when(consensus)
84+
.addRemotePeer(
85+
Mockito.eq(ConsensusManager.DEFAULT_CONSENSUS_GROUP_ID), Mockito.any(Peer.class));
86+
87+
Assert.assertThrows(
88+
AddPeerException.class,
89+
() -> newConsensusManager(consensus).addConfigNodePeer(newConfigNodeLocation(1)));
90+
}
91+
92+
@Test
93+
public void removeConfigNodePeerShouldIgnoreAlreadyRemovedPeer() throws Exception {
94+
final IConsensus consensus = Mockito.mock(IConsensus.class);
95+
Mockito.doThrow(
96+
new PeerNotInConsensusGroupException(
97+
ConsensusManager.DEFAULT_CONSENSUS_GROUP_ID, "127.0.0.1:10720"))
98+
.when(consensus)
99+
.removeRemotePeer(
100+
Mockito.eq(ConsensusManager.DEFAULT_CONSENSUS_GROUP_ID), Mockito.any(Peer.class));
101+
102+
Assert.assertTrue(
103+
newConsensusManager(consensus).removeConfigNodePeer(newConfigNodeLocation(1)));
104+
}
105+
106+
private static ConsensusManager newConsensusManager(final IConsensus consensus) throws Exception {
107+
return new ConsensusManager(Mockito.mock(IManager.class), consensus);
108+
}
109+
110+
private static TConfigNodeLocation newConfigNodeLocation(final int configNodeId) {
111+
return new TConfigNodeLocation(
112+
configNodeId,
113+
new TEndPoint("127.0.0.1", 10710 + configNodeId),
114+
new TEndPoint("127.0.0.1", 10720 + configNodeId));
115+
}
116+
}

iotdb-core/confignode/src/test/java/org/apache/iotdb/confignode/service/thrift/ConfigNodeRPCServiceProcessorTest.java

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,12 +27,15 @@
2727
import org.apache.iotdb.confignode.conf.ConfigNodeConfig;
2828
import org.apache.iotdb.confignode.consensus.response.datanode.DataNodeRegisterResp;
2929
import org.apache.iotdb.confignode.manager.ConfigManager;
30+
import org.apache.iotdb.confignode.manager.consensus.ConsensusManager;
3031
import org.apache.iotdb.confignode.rpc.thrift.TDataNodeRegisterReq;
3132
import org.apache.iotdb.confignode.rpc.thrift.TDataNodeRegisterResp;
3233
import org.apache.iotdb.confignode.rpc.thrift.TDataNodeRestartReq;
3334
import org.apache.iotdb.confignode.rpc.thrift.TDataNodeRestartResp;
3435
import org.apache.iotdb.confignode.rpc.thrift.TRuntimeConfiguration;
3536
import org.apache.iotdb.confignode.service.ConfigNode;
37+
import org.apache.iotdb.consensus.IConsensus;
38+
import org.apache.iotdb.consensus.exception.ConsensusGroupNotExistException;
3639
import org.apache.iotdb.rpc.TSStatusCode;
3740
import org.apache.iotdb.rpc.TimeoutChangeableTFastFramedTransport;
3841

@@ -161,4 +164,48 @@ public void testRestartDataNode() throws Exception {
161164
"1.2.3.4",
162165
sentRequest.getDataNodeConfiguration().getLocation().getClientRpcEndPoint().getIp());
163166
}
167+
168+
public void testDeleteConfigNodePeerShouldIgnoreMissingLocalPeer() throws Exception {
169+
CommonConfig commonConfig = Mockito.mock(CommonConfig.class);
170+
ConfigNodeConfig configNodeConfig = Mockito.mock(ConfigNodeConfig.class);
171+
ConfigNode configNode = Mockito.mock(ConfigNode.class);
172+
ConfigManager configManager = Mockito.mock(ConfigManager.class);
173+
ConsensusManager consensusManager = Mockito.mock(ConsensusManager.class);
174+
IConsensus consensus = Mockito.mock(IConsensus.class);
175+
Mockito.when(configManager.getConsensusManager()).thenReturn(consensusManager);
176+
Mockito.when(consensusManager.getConsensusGroupId())
177+
.thenReturn(ConsensusManager.DEFAULT_CONSENSUS_GROUP_ID);
178+
Mockito.when(consensusManager.getConsensusImpl()).thenReturn(consensus);
179+
Mockito.doThrow(
180+
new ConsensusGroupNotExistException(ConsensusManager.DEFAULT_CONSENSUS_GROUP_ID))
181+
.when(consensus)
182+
.deleteLocalPeer(ConsensusManager.DEFAULT_CONSENSUS_GROUP_ID);
183+
ConfigNodeRPCServiceProcessor sut =
184+
new ConfigNodeRPCServiceProcessor(
185+
commonConfig, configNodeConfig, configNode, configManager);
186+
187+
TSStatus status = sut.deleteConfigNodePeer(new TConfigNodeLocation());
188+
189+
Assert.assertEquals(TSStatusCode.SUCCESS_STATUS.getStatusCode(), status.getCode());
190+
Mockito.verify(configManager, Mockito.never()).getNodeManager();
191+
}
192+
193+
public void testDeleteConfigNodePeerShouldRejectMismatchedTarget() throws Exception {
194+
CommonConfig commonConfig = Mockito.mock(CommonConfig.class);
195+
ConfigNodeConfig configNodeConfig = Mockito.mock(ConfigNodeConfig.class);
196+
ConfigNode configNode = Mockito.mock(ConfigNode.class);
197+
ConfigManager configManager = Mockito.mock(ConfigManager.class);
198+
Mockito.when(configNodeConfig.getConfigNodeId()).thenReturn(2);
199+
ConfigNodeRPCServiceProcessor sut =
200+
new ConfigNodeRPCServiceProcessor(
201+
commonConfig, configNodeConfig, configNode, configManager);
202+
203+
TSStatus status =
204+
sut.deleteConfigNodePeer(
205+
new TConfigNodeLocation(
206+
1, new TEndPoint("127.0.0.1", 10710), new TEndPoint("127.0.0.1", 10720)));
207+
208+
Assert.assertEquals(TSStatusCode.REMOVE_CONFIGNODE_ERROR.getStatusCode(), status.getCode());
209+
Mockito.verify(configManager, Mockito.never()).getConsensusManager();
210+
}
164211
}

0 commit comments

Comments
 (0)