Skip to content

Commit 4b1d5e5

Browse files
authored
Fix ConfigNode NPE when extend/reconstruct region targets a non-DataNode id (#18075)
1 parent e9fe467 commit 4b1d5e5

5 files changed

Lines changed: 211 additions & 5 deletions

File tree

‎integration-test/src/test/java/org/apache/iotdb/confignode/it/regionmigration/pass/commit/IoTDBRegionGroupExpandAndShrinkForIoTV1IT.java‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import org.slf4j.LoggerFactory;
3737

3838
import java.sql.Connection;
39+
import java.sql.SQLException;
3940
import java.sql.Statement;
4041
import java.util.ArrayList;
4142
import java.util.List;
@@ -120,6 +121,64 @@ public void singleRegionTest() throws Exception {
120121
}
121122
}
122123

124+
/**
125+
* Regression test for TIMECHODB-0689: when the target id of "extend region" does not belong to
126+
* any registered DataNode (e.g. it is a ConfigNode id or simply does not exist), the ConfigNode
127+
* used to throw a NullPointerException in {@code checkExtendRegion} and the client only saw a
128+
* misleading "Fail to connect to any config node" message. After the fix a clear, actionable
129+
* error message must be returned instead.
130+
*/
131+
@Test
132+
public void extendRegionToInvalidDataNodeTest() throws Exception {
133+
EnvFactory.getEnv()
134+
.getConfig()
135+
.getCommonConfig()
136+
.setDataRegionConsensusProtocolClass(ConsensusFactory.IOT_CONSENSUS)
137+
.setSchemaRegionConsensusProtocolClass(ConsensusFactory.RATIS_CONSENSUS)
138+
.setDataReplicationFactor(1)
139+
.setSchemaReplicationFactor(1);
140+
141+
EnvFactory.getEnv().initClusterEnvironment(1, 3);
142+
143+
try (final Connection connection = makeItCloseQuietly(EnvFactory.getEnv().getConnection());
144+
final Statement statement = makeItCloseQuietly(connection.createStatement())) {
145+
// prepare data so that at least one region exists
146+
statement.execute(INSERTION1);
147+
statement.execute(FLUSH_COMMAND);
148+
149+
Map<Integer, Set<Integer>> regionMap = getAllRegionMap(statement);
150+
Set<Integer> allDataNodeId = getAllDataNodes(statement);
151+
Assert.assertFalse(regionMap.isEmpty());
152+
153+
int selectedRegion = regionMap.keySet().iterator().next();
154+
// an id that is guaranteed not to belong to any registered DataNode; a ConfigNode id triggers
155+
// the exact same code path (getRegisteredDataNode returns an empty configuration whose
156+
// location is null)
157+
int invalidDataNodeId = 9999;
158+
Assert.assertFalse(allDataNodeId.contains(invalidDataNodeId));
159+
160+
try {
161+
statement.execute(String.format(EXPAND_FORMAT, selectedRegion, invalidDataNodeId));
162+
Assert.fail("extend region to a non-existent DataNode is expected to fail");
163+
} catch (SQLException e) {
164+
String message = e.getMessage();
165+
LOGGER.info("extend region to invalid DataNode failed as expected: {}", message);
166+
Assert.assertNotNull(message);
167+
// the ConfigNode must not crash with an NPE any more ...
168+
Assert.assertFalse(
169+
"ConfigNode should not throw NullPointerException, but got: " + message,
170+
message.contains("NullPointerException"));
171+
// ... and the submission must be rejected cleanly. "extend region" wraps every region's
172+
// result, so the top-level message only reports the aggregate counts; the concrete "does
173+
// not
174+
// exist in the cluster" reason is carried in the per-region sub-status.
175+
Assert.assertTrue(
176+
"Expected the extend submission to be rejected but got: " + message,
177+
message.contains("failed to submit: 1"));
178+
}
179+
}
180+
}
181+
123182
private void regionGroupExpand(
124183
Statement statement,
125184
SyncConfigNodeIServiceClient client,

‎integration-test/src/test/java/org/apache/iotdb/confignode/it/regionmigration/pass/commit/IoTDBRegionReconstructForIoTV1IT.java‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545

4646
import java.io.File;
4747
import java.sql.Connection;
48+
import java.sql.SQLException;
4849
import java.sql.Statement;
4950
import java.util.Collections;
5051
import java.util.Map;
@@ -220,4 +221,58 @@ public void normal1C3DTest() throws Exception {
220221
}
221222
}
222223
}
224+
225+
/**
226+
* Regression test for TIMECHODB-0689 (reconstruct path): targeting "reconstruct region" at an id
227+
* that is not a registered DataNode (a ConfigNode id or a non-existent id) used to trigger a
228+
* NullPointerException in {@code checkReconstructRegion}. After the fix a clear, correct error
229+
* message must be returned instead of crashing the ConfigNode RPC.
230+
*/
231+
@Test
232+
public void reconstructRegionToInvalidDataNodeTest() throws Exception {
233+
EnvFactory.getEnv()
234+
.getConfig()
235+
.getCommonConfig()
236+
.setDataRegionConsensusProtocolClass(ConsensusFactory.IOT_CONSENSUS)
237+
.setSchemaRegionConsensusProtocolClass(ConsensusFactory.RATIS_CONSENSUS)
238+
.setDataReplicationFactor(1)
239+
.setSchemaReplicationFactor(1);
240+
241+
EnvFactory.getEnv().initClusterEnvironment(1, 3);
242+
243+
try (Connection connection = makeItCloseQuietly(EnvFactory.getEnv().getConnection());
244+
Statement statement = makeItCloseQuietly(connection.createStatement())) {
245+
// prepare data so that at least one region exists
246+
statement.execute(INSERTION1);
247+
statement.execute(FLUSH_COMMAND);
248+
249+
Map<Integer, Set<Integer>> regionMap = getAllRegionMap(statement);
250+
Set<Integer> allDataNodeId = getAllDataNodes(statement);
251+
Assert.assertFalse(regionMap.isEmpty());
252+
253+
int selectedRegion = regionMap.keySet().iterator().next();
254+
// an id that is guaranteed not to belong to any registered DataNode; a ConfigNode id triggers
255+
// the exact same code path (getRegisteredDataNode returns an empty configuration whose
256+
// location is null)
257+
int invalidDataNodeId = 9999;
258+
Assert.assertFalse(allDataNodeId.contains(invalidDataNodeId));
259+
260+
try {
261+
statement.execute(String.format(RECONSTRUCT_FORMAT, selectedRegion, invalidDataNodeId));
262+
Assert.fail("reconstruct region on a non-existent DataNode is expected to fail");
263+
} catch (SQLException e) {
264+
String message = e.getMessage();
265+
LOGGER.info("reconstruct region on invalid DataNode failed as expected: {}", message);
266+
Assert.assertNotNull(message);
267+
// the ConfigNode must not crash with an NPE any more ...
268+
Assert.assertFalse(
269+
"ConfigNode should not throw NullPointerException, but got: " + message,
270+
message.contains("NullPointerException"));
271+
// ... and the client should receive a clear, correct error message
272+
Assert.assertTrue(
273+
"Expected a 'does not exist in the cluster' error but got: " + message,
274+
message.contains("does not exist in the cluster"));
275+
}
276+
}
277+
}
223278
}

‎integration-test/src/test/java/org/apache/iotdb/confignode/it/removedatanode/IoTDBRemoveDataNodeNormalIT.java‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,12 +56,14 @@
5656
import java.util.concurrent.atomic.AtomicReference;
5757
import java.util.stream.Collectors;
5858

59+
import static org.apache.iotdb.confignode.it.regionmigration.IoTDBRegionOperationReliabilityITFramework.getAllRegionMap;
5960
import static org.apache.iotdb.confignode.it.regionmigration.IoTDBRegionOperationReliabilityITFramework.getDataRegionMap;
6061
import static org.apache.iotdb.confignode.it.removedatanode.IoTDBRemoveDataNodeUtils.awaitUntilSuccess;
6162
import static org.apache.iotdb.confignode.it.removedatanode.IoTDBRemoveDataNodeUtils.generateRemoveString;
6263
import static org.apache.iotdb.confignode.it.removedatanode.IoTDBRemoveDataNodeUtils.getConnectionWithSQLType;
6364
import static org.apache.iotdb.confignode.it.removedatanode.IoTDBRemoveDataNodeUtils.restartDataNodes;
6465
import static org.apache.iotdb.confignode.it.removedatanode.IoTDBRemoveDataNodeUtils.selectRemoveDataNodes;
66+
import static org.apache.iotdb.confignode.it.removedatanode.IoTDBRemoveDataNodeUtils.selectRemoveDataNodesWithoutRegionConflict;
6567
import static org.apache.iotdb.util.MagicUtils.makeItCloseQuietly;
6668

6769
@Category({ClusterIT.class})
@@ -286,8 +288,17 @@ public void testRemoveDataNode(
286288
allDataNodeId.add(result.getInt(ColumnHeaderConstant.NODE_ID));
287289
}
288290

289-
// Select data nodes to remove
290-
final Set<Integer> removeDataNodes = selectRemoveDataNodes(allDataNodeId, removeDataNodeNum);
291+
// Select data nodes to remove. When removing more than one DataNode we must avoid picking two
292+
// DataNodes that host replicas of the same consensus group, otherwise the
293+
// RemoveDataNodesProcedure would try to migrate two replicas of one group at once, which the
294+
// ConfigNode rejects ("Only one replica of the same consensus group is allowed to be migrated
295+
// at the same time."). Selecting purely at random therefore makes this test flaky, so use a
296+
// conflict-free selection.
297+
final Set<Integer> removeDataNodes =
298+
removeDataNodeNum > 1
299+
? selectRemoveDataNodesWithoutRegionConflict(
300+
allDataNodeId, removeDataNodeNum, getAllRegionMap(statement))
301+
: selectRemoveDataNodes(allDataNodeId, removeDataNodeNum);
291302

292303
List<DataNodeWrapper> removeDataNodeWrappers =
293304
removeDataNodes.stream()

‎integration-test/src/test/java/org/apache/iotdb/confignode/it/removedatanode/IoTDBRemoveDataNodeUtils.java‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,10 @@
3535
import java.sql.SQLException;
3636
import java.util.ArrayList;
3737
import java.util.Collections;
38+
import java.util.HashMap;
3839
import java.util.HashSet;
3940
import java.util.List;
41+
import java.util.Map;
4042
import java.util.Set;
4143
import java.util.concurrent.TimeUnit;
4244
import java.util.concurrent.atomic.AtomicReference;
@@ -73,6 +75,59 @@ public static Set<Integer> selectRemoveDataNodes(
7375
return new HashSet<>(shuffledDataNodeIds.subList(0, removeDataNodeNum));
7476
}
7577

78+
/**
79+
* Select {@code removeDataNodeNum} DataNodes to remove such that no two of them host a replica of
80+
* the same consensus group. Removing two DataNodes that share a region group would make the
81+
* generated {@code RemoveDataNodesProcedure} try to migrate two replicas of the same group at
82+
* once, which the ConfigNode rejects ("Only one replica of the same consensus group is allowed to
83+
* be migrated at the same time."). Randomly picking DataNodes (see {@link
84+
* #selectRemoveDataNodes}) therefore makes multi-DataNode-remove tests flaky; this method keeps
85+
* the selection valid and deterministic-enough for such tests.
86+
*
87+
* @param allDataNodeId all registered DataNode ids
88+
* @param removeDataNodeNum how many DataNodes to remove
89+
* @param regionMap regionId -&gt; the set of DataNode ids hosting a replica of that region group
90+
* (all region types)
91+
* @return the selected DataNode ids, or throws if no conflict-free selection of the requested
92+
* size exists
93+
*/
94+
public static Set<Integer> selectRemoveDataNodesWithoutRegionConflict(
95+
Set<Integer> allDataNodeId, int removeDataNodeNum, Map<Integer, Set<Integer>> regionMap) {
96+
// dataNodeId -> the set of region groups it hosts a replica of
97+
Map<Integer, Set<Integer>> dataNodeToRegions = new HashMap<>();
98+
for (Integer dataNodeId : allDataNodeId) {
99+
dataNodeToRegions.put(dataNodeId, new HashSet<>());
100+
}
101+
regionMap.forEach(
102+
(regionId, dataNodeIds) ->
103+
dataNodeIds.forEach(
104+
dataNodeId ->
105+
dataNodeToRegions
106+
.computeIfAbsent(dataNodeId, id -> new HashSet<>())
107+
.add(regionId)));
108+
109+
List<Integer> shuffledDataNodeIds = new ArrayList<>(allDataNodeId);
110+
Collections.shuffle(shuffledDataNodeIds);
111+
112+
Set<Integer> selected = new HashSet<>();
113+
Set<Integer> coveredRegions = new HashSet<>();
114+
for (Integer dataNodeId : shuffledDataNodeIds) {
115+
Set<Integer> regions = dataNodeToRegions.getOrDefault(dataNodeId, Collections.emptySet());
116+
if (Collections.disjoint(coveredRegions, regions)) {
117+
selected.add(dataNodeId);
118+
coveredRegions.addAll(regions);
119+
if (selected.size() == removeDataNodeNum) {
120+
return selected;
121+
}
122+
}
123+
}
124+
throw new IllegalStateException(
125+
String.format(
126+
"Cannot select %d DataNodes to remove without a same-region-group conflict. "
127+
+ "allDataNodeId=%s, regionMap=%s",
128+
removeDataNodeNum, allDataNodeId, regionMap));
129+
}
130+
76131
public static void restartDataNodes(List<DataNodeWrapper> dataNodeWrappers) {
77132
dataNodeWrappers.parallelStream()
78133
.forEach(

‎iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/ProcedureManager.java‎

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -881,7 +881,6 @@ private TSStatus checkReconstructRegion(
881881
new Pair<>("Coordinator", coordinator)),
882882
req.getModel());
883883

884-
ConfigNodeConfig conf = ConfigNodeDescriptor.getInstance().getConf();
885884
if (configManager
886885
.getPartitionManager()
887886
.getAllReplicaSetsMap(regionId.getType())
@@ -1245,10 +1244,29 @@ private TSStatus migrateOneRegion(
12451244
return new TSStatus(TSStatusCode.SUCCESS_STATUS.getStatusCode());
12461245
}
12471246

1247+
/**
1248+
* Resolve the location of a registered DataNode, or return {@code null} if the given id does not
1249+
* belong to any registered DataNode (e.g. it is a ConfigNode id or simply does not exist). {@link
1250+
* org.apache.iotdb.confignode.manager.node.NodeManager#getRegisteredDataNode} returns an empty
1251+
* {@link TDataNodeConfiguration} whose location is {@code null} in that case, so callers must not
1252+
* dereference the result blindly.
1253+
*/
1254+
private TDataNodeLocation getRegisteredDataNodeLocationOrNull(int dataNodeId) {
1255+
return configManager.getNodeManager().getRegisteredDataNode(dataNodeId).getLocation();
1256+
}
1257+
12481258
public TSStatus reconstructRegion(TReconstructRegionReq req) {
12491259
RegionMaintainHandler handler = env.getRegionMaintainHandler();
12501260
final TDataNodeLocation targetDataNode =
1251-
configManager.getNodeManager().getRegisteredDataNode(req.getDataNodeId()).getLocation();
1261+
getRegisteredDataNodeLocationOrNull(req.getDataNodeId());
1262+
if (targetDataNode == null) {
1263+
// The target id is not a registered DataNode. Reject here instead of pushing a null down into
1264+
// checkReconstructRegion, which would otherwise throw a NullPointerException.
1265+
return new TSStatus(TSStatusCode.RECONSTRUCT_REGION_ERROR.getStatusCode())
1266+
.setMessage(
1267+
String.format(
1268+
"Target DataNode %s does not exist in the cluster", req.getDataNodeId()));
1269+
}
12521270
try (AutoCloseableLock ignoredLock =
12531271
AutoCloseableLock.acquire(env.getSubmitRegionMigrateLock())) {
12541272
List<ReconstructRegionProcedure> procedures = new ArrayList<>();
@@ -1352,7 +1370,15 @@ private TSStatus extendOneRegion(int theRegionId, TExtendRegionReq req) {
13521370

13531371
// find target dn
13541372
final TDataNodeLocation targetDataNode =
1355-
configManager.getNodeManager().getRegisteredDataNode(req.getDataNodeId()).getLocation();
1373+
getRegisteredDataNodeLocationOrNull(req.getDataNodeId());
1374+
if (targetDataNode == null) {
1375+
// The target id is not a registered DataNode. Reject here instead of pushing a null down
1376+
// into checkExtendRegion, which would otherwise throw a NullPointerException.
1377+
return new TSStatus(TSStatusCode.EXTEND_REGION_ERROR.getStatusCode())
1378+
.setMessage(
1379+
String.format(
1380+
"Target DataNode %s does not exist in the cluster", req.getDataNodeId()));
1381+
}
13561382
// select coordinator for adding peer
13571383
RegionMaintainHandler handler = env.getRegionMaintainHandler();
13581384
// TODO: choose the DataNode which has lowest load

0 commit comments

Comments
 (0)