You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by "HxpSerein (via GitHub)" <gi...@apache.org> on 2024/10/17 11:23:10 UTC
[PR] Enhance Remove DataNode Test [iotdb]
HxpSerein opened a new pull request, #13809:
URL: https://github.com/apache/iotdb/pull/13809
## Description
Improve remove datanode related UT/IT for complex test scenarios in region migration tests.
- Task management and conflict checking
- Running/ReadOnly nodes can be rejoined as new nodes after being removed
- Nodes and their region statuses that failed to remove are correctly rolled back
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhance Remove DataNode Test [iotdb]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum merged PR #13809:
URL: https://github.com/apache/iotdb/pull/13809
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhance Remove DataNode Test [iotdb]
Posted by "HxpSerein (via GitHub)" <gi...@apache.org>.
HxpSerein commented on code in PR #13809:
URL: https://github.com/apache/iotdb/pull/13809#discussion_r1814519866
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/load/cache/region/RegionCache.java:
##########
@@ -65,7 +65,7 @@ public synchronized void cacheHeartbeatSample(
return;
}
RegionStatus lastStatus = ((RegionHeartbeatSample) getLastSample()).getStatus();
- if (lastStatus.equals(RegionStatus.Adding) || lastStatus.equals(RegionStatus.Removing)) {
Review Comment:
fixed
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhance Remove DataNode Test [iotdb]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #13809:
URL: https://github.com/apache/iotdb/pull/13809#discussion_r1804717218
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/load/cache/region/RegionCache.java:
##########
@@ -65,7 +65,7 @@ public synchronized void cacheHeartbeatSample(
return;
}
RegionStatus lastStatus = ((RegionHeartbeatSample) getLastSample()).getStatus();
- if (lastStatus.equals(RegionStatus.Adding) || lastStatus.equals(RegionStatus.Removing)) {
Review Comment:
same as above. This change may occur some bugs
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/load/cache/node/DataNodeHeartbeatCache.java:
##########
@@ -38,10 +38,6 @@ public DataNodeHeartbeatCache(int dataNodeId) {
@Override
public synchronized void updateCurrentStatistics() {
- // The Removing status can not be updated
Review Comment:
Why remove this line?Then the remove status maybe updated by Running or Readonly or Unknown?
##########
integration-test/src/test/java/org/apache/iotdb/confignode/it/removedatanode/IoTDBRemoveDataNodeITFramework.java:
##########
@@ -88,7 +89,8 @@ public void successTest(
final int configNodeNum,
final int dataNodeNum,
final int removeDataNodeNum,
- final int dataRegionPerDataNode)
+ final int dataRegionPerDataNode,
+ final boolean needRestartRemoveDataNode)
Review Comment:
rejoinRemovedDataNode?
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhance Remove DataNode Test [iotdb]
Posted by "HxpSerein (via GitHub)" <gi...@apache.org>.
HxpSerein commented on code in PR #13809:
URL: https://github.com/apache/iotdb/pull/13809#discussion_r1814953830
##########
integration-test/src/test/java/org/apache/iotdb/confignode/it/removedatanode/IoTDBRemoveDataNodeITFramework.java:
##########
@@ -88,7 +89,8 @@ public void successTest(
final int configNodeNum,
final int dataNodeNum,
final int removeDataNodeNum,
- final int dataRegionPerDataNode)
+ final int dataRegionPerDataNode,
+ final boolean needRestartRemoveDataNode)
Review Comment:
renamed
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhance Remove DataNode Test [iotdb]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #13809:
URL: https://github.com/apache/iotdb/pull/13809#discussion_r1818336683
##########
iotdb-protocol/thrift-datanode/src/main/thrift/datanode.thrift:
##########
@@ -804,7 +808,7 @@ service IDataNodeRPCService {
/**
* Config node will stop the Data node.
*/
- common.TSStatus stopDataNode()
+ common.TSStatus stopDataNode(TStopDataNodeReq req)
Review Comment:
It appears that currently, this interface is only called when removing a datanode, and its parameter is always set to true. Can we assume this is a fixed behavior and directly proceed to delete the properties file? If, in the future, there’s a need to stop a node without deleting the properties file, we can then extend this interface. Additionally, its semantics now seem to be more than just stopping a node; perhaps it should be called stopAndClearDatanode?
##########
integration-test/src/test/java/org/apache/iotdb/confignode/it/removedatanode/IoTDBRemoveDataNodeITFramework.java:
##########
@@ -204,6 +214,16 @@ public void testRemoveDataNode(
}
LOGGER.info("Remove DataNodes success");
+
+ if (rejoinRemovedDataNode) {
+ try {
+ removeDataNodeWrappers.parallelStream().forEach(DataNodeWrapper::start);
+ LOGGER.info("RemoveDataNodes:{} rejoined successfully.", removeDataNodes);
Review Comment:
check rejoin result？
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/load/LoadManager.java:
##########
@@ -282,7 +282,7 @@ public void forceUpdateNodeCache(
*/
public void removeNodeCache(int nodeId) {
loadCache.removeNodeCache(nodeId);
- loadCache.updateNodeStatistics();
+ loadCache.updateNodeStatistics(false);
Review Comment:
It looks like this is also an active update, not a heartbeat update, maybe set it to true?
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/impl/node/RemoveDataNodesProcedure.java:
##########
@@ -188,17 +189,20 @@ private void checkRegionStatusAndStopDataNode(ConfigNodeProcedureEnv env) {
"{}, Region all migrated successfully, start to stop DataNode: {}",
Review Comment:
move logs to line 196?
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/load/LoadManager.java:
##########
@@ -282,7 +282,7 @@ public void forceUpdateNodeCache(
*/
public void removeNodeCache(int nodeId) {
loadCache.removeNodeCache(nodeId);
- loadCache.updateNodeStatistics();
+ loadCache.updateNodeStatistics(false);
Review Comment:
Why set here to false?
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhance Remove DataNode Test [iotdb]
Posted by "HxpSerein (via GitHub)" <gi...@apache.org>.
HxpSerein commented on code in PR #13809:
URL: https://github.com/apache/iotdb/pull/13809#discussion_r1814518950
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/load/cache/node/DataNodeHeartbeatCache.java:
##########
@@ -38,10 +38,6 @@ public DataNodeHeartbeatCache(int dataNodeId) {
@Override
public synchronized void updateCurrentStatistics() {
- // The Removing status can not be updated
Review Comment:
fixed
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org