You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by "Caideyipi (via GitHub)" <gi...@apache.org> on 2026/05/06 08:57:54 UTC
[PR] Fixed multiple configNode bugs [iotdb]
Caideyipi opened a new pull request, #17609:
URL: https://github.com/apache/iotdb/pull/17609
## Description
1. In Simple mode, a successful response will be returned stating "Metadata has taken effect, but WAL has not been flushed to disk." Configuration changes will be lost after a restart.
2. In the Simple mode, some files have the same sorting key, posing a risk of log replay in disorder.
3. The return TSStatus of ConsensusManager.write() is not checked, and the exception is only a warn followed by proceeding to the success branch. This means that if consensus writing is rejected, redirected, or the state machine returns a failure, the caller will still receive ACCEPT, while the local heartbeat cache/metrics have already been created in advance, leading to a divergence between metadata and runtime states.
4. The value returned by getRegionGroupCount() increases by 1 every time it is counted, and this value is used for capacity expansion judgment, display, and maxRegionGroupNum calculation, which directly affects the region scaling decision.
5. When taking a snapshot, the regionMaintainTaskList lock is not acquired before traversing. Concurrent offer/poll may throw ConcurrentModificationException or put the maintenance queue snapshot into an inconsistent state.
6. If the pipe listener fails to process the snapshot, the entire takeSnapshot() method will return false; however, ConfigRegionStateMachine combines and absorbs the failures of loadSnapshot and the pipe listener, which can amplify auxiliary link failures into consensus snapshot failures, and even mask the real failure of snapshot loading.
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error
conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition,
design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design
(or naming) decision point and compare the alternatives with the designs that you've implemented
(or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere
(e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list),
link to that discussion from this PR description and explain what have changed in your final design
compared to your original proposal or the consensus version in the end of the discussion.
If something hasn't changed since the original discussion, you can omit a detailed discussion of
those aspects of the design here, perhaps apart from brief mentioning for the sake of readability
of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] concurrent read
- [ ] concurrent write
- [ ] concurrent read and write
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods.
- [ ] added or updated version, __license__, or notice information
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious
for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold
for code coverage.
- [ ] added integration tests.
- [ ] been tested in a test IoTDB cluster.
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items
apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items
from the checklist above are strictly necessary, but it would be very helpful if you at least
self-review the PR. -->
<hr>
##### Key changed/added classes (or packages if there are too many classes) in this PR
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#issuecomment-5139230458
## [](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17609) **Quality Gate failed**
Failed conditions
 [C Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17609) (required ≥ A)
<!-- sqra:qg_revision=d936d83775b7ae4e05622e21bf7cf0ab22cc6891d98f4af00b10c47a75cba2db -->
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17609)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "Caideyipi (via GitHub)" <gi...@apache.org>.
Caideyipi commented on code in PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#discussion_r3680780440
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:
##########
@@ -121,6 +121,13 @@ public TSStatus write(IConsensusRequest request) {
/** Transmit {@link ConfigPhysicalPlan} to {@link ConfigPlanExecutor} */
protected TSStatus write(ConfigPhysicalPlan plan) {
+ if (ConsensusFactory.SIMPLE_CONSENSUS.equals(CONF.getConfigNodeConsensusProtocolClass())) {
+ final TSStatus persistStatus = persistPlanForSimpleConsensus(plan);
+ if (persistStatus.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
+ return persistStatus;
+ }
+ }
Review Comment:
Addressed in cddb4b1c962. A plan whose execution fails is now removed from the SimpleConsensus WAL by truncating and force-syncing back to the pre-write offset, so normal rollback leaves nothing to replay. `ConfigRegionStateMachineTest.testFailedSimpleConsensusWriteRollsBackPersistedPlan` verifies the WAL contains no replayable plan. For the registration-related plans touched here, `NodeInfoTest.testRegistrationPlansAreIdempotentForWalReplay` also executes RegisterDataNodePlan, ApplyConfigNodePlan, and UpdateVersionInfoPlan twice and verifies stable state. If truncation itself fails, the status and operator log explicitly warn that replay remains possible.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:
##########
@@ -351,56 +360,48 @@ public boolean isReadOnly() {
return CommonDescriptor.getInstance().getConfig().isReadOnly();
}
- private void writeLogForSimpleConsensus(ConfigPhysicalPlan plan) {
- if (simpleLogFile.length() > LOG_FILE_MAX_SIZE) {
- try {
- simpleLogWriter.force();
- File completedFilePath = new File(FILE_PATH + startIndex + "_" + endIndex);
- Files.move(
- simpleLogFile.toPath(), completedFilePath.toPath(), StandardCopyOption.ATOMIC_MOVE);
- } catch (IOException e) {
- LOGGER.error("Can't force logWriter for ConfigNode SimpleConsensus mode", e);
+ private TSStatus persistPlanForSimpleConsensus(ConfigPhysicalPlan plan) {
+ try {
+ if (simpleLogWriter == null || simpleLogFile == null) {
+ throw new IOException("SimpleConsensus log writer is not initialized.");
}
- for (int retry = 0; retry < 5; retry++) {
- try {
- simpleLogWriter.close();
- } catch (IOException e) {
- LOGGER.warn(
- "Can't close StandAloneLog for ConfigNode SimpleConsensus mode, "
- + "filePath: {}, retry: {}",
- simpleLogFile.getAbsolutePath(),
- retry);
- try {
- // Sleep 1s and retry
- TimeUnit.SECONDS.sleep(1);
- } catch (InterruptedException e2) {
- Thread.currentThread().interrupt();
- LOGGER.warn("Unexpected interruption during the close method of logWriter");
- }
- continue;
- }
- break;
+
+ if (simpleLogFile.length() > LOG_FILE_MAX_SIZE) {
+ rollSimpleConsensusLogFile();
}
- startIndex = endIndex + 1;
- createLogFile(startIndex);
- }
- try {
ByteBuffer buffer = plan.serializeToByteBuffer();
buffer.position(buffer.limit());
simpleLogWriter.write(buffer);
+ simpleLogWriter.force();
Review Comment:
Addressed. The scheduled flush executor and `flushWALForSimpleConsensus` method were removed; each persisted write is synchronously forced, and the writer is now closed from `stop()`.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:
##########
@@ -351,56 +360,48 @@ public boolean isReadOnly() {
return CommonDescriptor.getInstance().getConfig().isReadOnly();
}
- private void writeLogForSimpleConsensus(ConfigPhysicalPlan plan) {
- if (simpleLogFile.length() > LOG_FILE_MAX_SIZE) {
- try {
- simpleLogWriter.force();
- File completedFilePath = new File(FILE_PATH + startIndex + "_" + endIndex);
- Files.move(
- simpleLogFile.toPath(), completedFilePath.toPath(), StandardCopyOption.ATOMIC_MOVE);
- } catch (IOException e) {
- LOGGER.error("Can't force logWriter for ConfigNode SimpleConsensus mode", e);
+ private TSStatus persistPlanForSimpleConsensus(ConfigPhysicalPlan plan) {
+ try {
+ if (simpleLogWriter == null || simpleLogFile == null) {
+ throw new IOException("SimpleConsensus log writer is not initialized.");
}
- for (int retry = 0; retry < 5; retry++) {
- try {
- simpleLogWriter.close();
- } catch (IOException e) {
- LOGGER.warn(
- "Can't close StandAloneLog for ConfigNode SimpleConsensus mode, "
- + "filePath: {}, retry: {}",
- simpleLogFile.getAbsolutePath(),
- retry);
- try {
- // Sleep 1s and retry
- TimeUnit.SECONDS.sleep(1);
- } catch (InterruptedException e2) {
- Thread.currentThread().interrupt();
- LOGGER.warn("Unexpected interruption during the close method of logWriter");
- }
- continue;
- }
- break;
+
+ if (simpleLogFile.length() > LOG_FILE_MAX_SIZE) {
+ rollSimpleConsensusLogFile();
}
- startIndex = endIndex + 1;
- createLogFile(startIndex);
- }
- try {
ByteBuffer buffer = plan.serializeToByteBuffer();
buffer.position(buffer.limit());
simpleLogWriter.write(buffer);
+ simpleLogWriter.force();
endIndex = endIndex + 1;
} catch (Exception e) {
LOGGER.error(
- "Can't serialize current ConfigPhysicalPlan for ConfigNode SimpleConsensus mode", e);
+ "Persist current ConfigPhysicalPlan for ConfigNode SimpleConsensus mode failed", e);
+ return new TSStatus(TSStatusCode.EXECUTE_STATEMENT_ERROR.getStatusCode())
+ .setMessage(
+ "Persist ConfigNode SimpleConsensus log failed: " + String.valueOf(e.getMessage()));
Review Comment:
Addressed in the rewritten persistence error path: the message now directly concatenates `e.getMessage()`.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/persistence/partition/PartitionInfo.java:
##########
@@ -1008,9 +1010,14 @@ public boolean processTakeSnapshot(File snapshotDir) throws TException, IOExcept
databasePartitionTableEntry.getValue().serialize(bufferedOutputStream, protocol);
}
+ final List<RegionMaintainTask> copiedRegionMaintainTaskList;
+ synchronized (regionMaintainTaskList) {
+ copiedRegionMaintainTaskList = new ArrayList<>(regionMaintainTaskList);
+ }
Review Comment:
Thanks for noting this. The snapshot path intentionally copies under the lock to obtain a consistent list, then performs serialization after releasing the lock. As noted, this list is expected to remain small for ConfigNode.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/persistence/partition/DatabasePartitionTable.java:
##########
@@ -243,7 +243,7 @@ public int getRegionGroupCount(TConsensusGroupType type) {
result.getAndIncrement();
}
});
- return result.getAndIncrement();
+ return result.get();
Review Comment:
Yes, this was an off-by-one side effect: `getAndIncrement()` returned the correct count but unnecessarily advanced the local counter before returning. It now uses `get()`, and `PartitionInfoTest.testRegionGroupCount` covers both schema and data region counts.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:
##########
@@ -122,16 +122,32 @@ public TSStatus write(IConsensusRequest request) {
/** Transmit {@link ConfigPhysicalPlan} to {@link ConfigPlanExecutor} */
protected TSStatus write(ConfigPhysicalPlan plan) {
+ SimpleConsensusPersistResult persistResult = null;
+ if (ConsensusFactory.SIMPLE_CONSENSUS.equals(CONF.getConfigNodeConsensusProtocolClass())) {
+ persistResult = persistPlanForSimpleConsensus(plan);
Review Comment:
Addressed. The failed-write test audits the persist-success/execute-failure path and verifies rollback leaves no replayable WAL entry. I also added replay idempotency coverage for the plan types directly touched by the registration changes: RegisterDataNodePlan, ApplyConfigNodePlan, and UpdateVersionInfoPlan.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:
##########
@@ -122,16 +122,32 @@ public TSStatus write(IConsensusRequest request) {
/** Transmit {@link ConfigPhysicalPlan} to {@link ConfigPlanExecutor} */
protected TSStatus write(ConfigPhysicalPlan plan) {
+ SimpleConsensusPersistResult persistResult = null;
+ if (ConsensusFactory.SIMPLE_CONSENSUS.equals(CONF.getConfigNodeConsensusProtocolClass())) {
+ persistResult = persistPlanForSimpleConsensus(plan);
+ final TSStatus persistStatus = persistResult.status;
+ if (persistStatus.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
+ return persistStatus;
+ }
+ }
+
TSStatus result;
try {
result = executor.executeNonQueryPlan(plan);
Review Comment:
Addressed. Rollback failure logging now includes the plan type, WAL file, truncate offset, and pre-write end index. The returned status also explicitly says that the persisted plan may replay after restart, so both operators and callers can see the possible WAL/in-memory divergence.
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#issuecomment-4516018781
## [](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17609) **Quality Gate passed**
Issues
 [6 New issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=17609&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=17609&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_iotdb&pullRequest=17609&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=17609&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=17609&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17609)
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#issuecomment-5126170831
## [](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17609) **Quality Gate failed**
Failed conditions
 [C Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17609) (required ≥ A)
<!-- sqra:qg_revision=4943a8f656717916e3bc08b5307bde5080da7650d2a79f02a44122673bb5f723 -->
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17609)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#issuecomment-4516161594
## [](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17609) **Quality Gate passed**
Issues
 [6 New issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=17609&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=17609&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_iotdb&pullRequest=17609&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=17609&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=17609&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17609)
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "Caideyipi (via GitHub)" <gi...@apache.org>.
Caideyipi commented on code in PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#discussion_r4309969142
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:
##########
@@ -579,60 +602,84 @@ public boolean isReadOnly() {
return CommonDescriptor.getInstance().getConfig().isReadOnly();
}
- private void writeLogForSimpleConsensus(ConfigPhysicalPlan plan) {
- if (simpleLogFile.length() > LOG_FILE_MAX_SIZE) {
- try {
- simpleLogWriter.force();
- File completedFilePath = new File(FILE_PATH + startIndex + "_" + endIndex);
- Files.move(
- simpleLogFile.toPath(), completedFilePath.toPath(), StandardCopyOption.ATOMIC_MOVE);
- } catch (IOException e) {
- LOGGER.error(
- ConfigNodeMessages.CAN_T_FORCE_LOGWRITER_FOR_CONFIGNODE_SIMPLECONSENSUS_MODE, e);
+ private SimpleConsensusPersistResult persistPlanForSimpleConsensus(ConfigPhysicalPlan plan) {
+ File persistedLogFile = null;
+ long logFileSizeBeforeWrite = 0;
+ int endIndexBeforeWrite = endIndex;
+ try {
+ if (simpleLogWriter == null || simpleLogFile == null) {
+ throw new IOException(ConfigNodeMessages.SIMPLECONSENSUS_LOG_WRITER_IS_NOT_INITIALIZED);
}
- for (int retry = 0; retry < 5; retry++) {
- try {
- simpleLogWriter.close();
- } catch (IOException e) {
- LOGGER.warn(
- ConfigNodeMessages
- .LOG_CONFIGREGION_UPDATE_FAILED_TO_CLOSE_THE_CONFIGNODE_SIMPLECONSENSUS_LOG_FILEPATH_ARG_RETRY_ARG_188A702E,
- simpleLogFile.getAbsolutePath(),
- retry);
- try {
- // Sleep 1s and retry
- TimeUnit.SECONDS.sleep(1);
- } catch (InterruptedException e2) {
- Thread.currentThread().interrupt();
- LOGGER.warn(
- ConfigNodeMessages.UNEXPECTED_INTERRUPTION_DURING_THE_CLOSE_METHOD_OF_LOGWRITER);
- }
- continue;
- }
- break;
+
+ if (simpleLogFile.length() > LOG_FILE_MAX_SIZE) {
+ rollSimpleConsensusLogFile();
}
- startIndex = endIndex + 1;
- createLogFile(startIndex);
- }
- try {
+ persistedLogFile = simpleLogFile;
+ logFileSizeBeforeWrite = persistedLogFile.length();
+ endIndexBeforeWrite = endIndex;
+
ByteBuffer buffer = plan.serializeToByteBuffer();
buffer.position(buffer.limit());
simpleLogWriter.write(buffer);
+ simpleLogWriter.force();
endIndex = endIndex + 1;
} catch (Exception e) {
LOGGER.error(
ConfigNodeMessages
- .CAN_T_SERIALIZE_CURRENT_CONFIGPHYSICALPLAN_FOR_CONFIGNODE_SIMPLECONSENSUS_MODE,
+ .PERSIST_CURRENT_CONFIGPHYSICALPLAN_FOR_CONFIGNODE_SIMPLECONSENSUS_MODE_FAILED,
e);
+ return SimpleConsensusPersistResult.failure(
Review Comment:
Fixed in [1e1ffe04c8d](https://github.com/apache/iotdb/commit/1e1ffe04c8d035f46815ac91334a0d0183e9a913). Append and `force()` failures now restore the saved pre-write WAL offset and force-sync the truncation before another write is accepted. If cleanup cannot complete, further SimpleConsensus writes are rejected until restart, with an explicit status and operator log.
Regression tests cover a complete appended record followed by a force failure, a partial append failure, and cleanup failure, in addition to the existing executor-failure test. The replay checks preserve the preceding accepted plan, exclude the failed plan, and verify that a subsequent write succeeds after successful cleanup.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/node/NodeManager.java:
##########
@@ -1385,6 +1412,144 @@ public TDataNodeLocation getLowestLoadDataNode(Set<Integer> nodes) {
return getRegisteredDataNode(dataNodeId).getLocation();
}
+ private TSStatus writeConfigPhysicalPlan(ConfigPhysicalPlan plan) {
+ try {
+ return getConsensusManager().write(plan);
+ } catch (ConsensusException e) {
+ LOGGER.warn(CONSENSUS_WRITE_ERROR, e);
+ return new TSStatus(TSStatusCode.EXECUTE_STATEMENT_ERROR.getStatusCode())
+ .setMessage(e.getMessage());
+ }
+ }
+
+ private boolean isConsensusWriteSuccessful(TSStatus status) {
+ return status != null && status.getCode() == TSStatusCode.SUCCESS_STATUS.getStatusCode();
+ }
+
+ private TSStatus rollbackDataNodeRegistration(
+ TDataNodeLocation dataNodeLocation, TSStatus versionUpdateStatus) {
+ final TSStatus rollbackStatus =
+ writeConfigPhysicalPlan(
+ new RemoveDataNodePlan(Collections.singletonList(dataNodeLocation)));
+ final String failureMessage =
+ String.format(
+ "Failed to persist version info for DataNode %d: %s",
+ dataNodeLocation.getDataNodeId(), describeStatus(versionUpdateStatus));
+ if (isConsensusWriteSuccessful(rollbackStatus)) {
+ return buildStatus(
+ versionUpdateStatus.getCode(),
+ failureMessage,
+ "The registration has been rolled back. Please retry the registration.");
+ }
+
+ LOGGER.error(
+ "Failed to roll back DataNode registration {} after version info persistence failure. "
+ + ROLLBACK_STATUS_LOG_FORMAT,
+ dataNodeLocation,
+ versionUpdateStatus,
+ rollbackStatus);
+ return buildStatus(
+ rollbackStatus.getCode(),
+ failureMessage,
+ String.format(REGISTRATION_ROLLBACK_FAILED_FORMAT, describeStatus(rollbackStatus)),
+ MANUAL_REGISTRATION_CLEANUP_MESSAGE);
+ }
+
+ private TSStatus rollbackAiNodeRegistration(
+ TAINodeLocation aiNodeLocation, TSStatus versionUpdateStatus) {
+ final TSStatus rollbackStatus = writeConfigPhysicalPlan(new RemoveAINodePlan(aiNodeLocation));
+ final String failureMessage =
+ String.format(
+ "Failed to persist version info for AINode %d: %s",
+ aiNodeLocation.getAiNodeId(), describeStatus(versionUpdateStatus));
+ if (isConsensusWriteSuccessful(rollbackStatus)) {
+ return buildStatus(
+ versionUpdateStatus.getCode(),
+ failureMessage,
+ "The registration has been rolled back. Please retry the registration.");
+ }
+
+ LOGGER.error(
+ "Failed to roll back AINode registration {} after version info persistence failure. "
+ + ROLLBACK_STATUS_LOG_FORMAT,
+ aiNodeLocation,
+ versionUpdateStatus,
+ rollbackStatus);
+ return buildStatus(
+ rollbackStatus.getCode(),
+ failureMessage,
+ String.format(REGISTRATION_ROLLBACK_FAILED_FORMAT, describeStatus(rollbackStatus)),
+ MANUAL_REGISTRATION_CLEANUP_MESSAGE);
+ }
+
+ private TSStatus rollbackConfigNodeRegistration(
+ TConfigNodeLocation configNodeLocation, TSStatus versionUpdateStatus) {
+ final TSStatus rollbackStatus =
+ writeConfigPhysicalPlan(new RemoveConfigNodePlan(configNodeLocation));
Review Comment:
Fixed in [1e1ffe04c8d](https://github.com/apache/iotdb/commit/1e1ffe04c8d035f46815ac91334a0d0183e9a913). A failed ConfigNode version write no longer submits `RemoveConfigNodePlan`. Since `ADD_PEER` has already joined the node to Raft, its membership is retained and the failure is propagated so `REGISTER_SUCCESS` can retry the idempotent registration and version plans. Heartbeat-cache creation remains after both writes succeed.
The regression test applies the plans through a real `NodeInfo` as the joining node. It verifies that the node remains Running after the version-write failure and after a successful retry, registration contains one member, the version is updated, and no removal plan is written.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/node/NodeManager.java:
##########
@@ -1385,6 +1412,144 @@ public TDataNodeLocation getLowestLoadDataNode(Set<Integer> nodes) {
return getRegisteredDataNode(dataNodeId).getLocation();
}
+ private TSStatus writeConfigPhysicalPlan(ConfigPhysicalPlan plan) {
+ try {
+ return getConsensusManager().write(plan);
+ } catch (ConsensusException e) {
+ LOGGER.warn(CONSENSUS_WRITE_ERROR, e);
+ return new TSStatus(TSStatusCode.EXECUTE_STATEMENT_ERROR.getStatusCode())
+ .setMessage(e.getMessage());
+ }
+ }
+
+ private boolean isConsensusWriteSuccessful(TSStatus status) {
+ return status != null && status.getCode() == TSStatusCode.SUCCESS_STATUS.getStatusCode();
+ }
+
+ private TSStatus rollbackDataNodeRegistration(
+ TDataNodeLocation dataNodeLocation, TSStatus versionUpdateStatus) {
+ final TSStatus rollbackStatus =
+ writeConfigPhysicalPlan(
+ new RemoveDataNodePlan(Collections.singletonList(dataNodeLocation)));
+ final String failureMessage =
+ String.format(
+ "Failed to persist version info for DataNode %d: %s",
+ dataNodeLocation.getDataNodeId(), describeStatus(versionUpdateStatus));
Review Comment:
Fixed in [1e1ffe04c8d](https://github.com/apache/iotdb/commit/1e1ffe04c8d035f46815ac91334a0d0183e9a913). The registration failure statuses, rollback logs, exception templates, and fallback status descriptions now use matching English and Chinese `ConfigNodeMessages` constants. Each outcome uses a complete format template, and the raw English fragment constants were removed. The new WAL failure messages are also localized.
Full-reactor test compilation passed for both locales; all 431 ConfigNode message keys and placeholder counts match. A scan found no added raw English literals in main Java, and all 12 focused WAL/registration tests passed in both locales.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/persistence/partition/DatabasePartitionTable.java:
##########
@@ -243,7 +243,7 @@ public int getRegionGroupCount(TConsensusGroupType type) {
result.getAndIncrement();
}
});
- return result.getAndIncrement();
+ return result.get();
Review Comment:
Correction to my earlier reply: `result` is a local counter, and `getAndIncrement()` returns its value before incrementing it. Replacing it with `get()` leaves the returned count unchanged; this removes an unused increment rather than fixing an off-by-one bug. I have corrected item 4 in the PR description accordingly.
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "CRZbulabula (via GitHub)" <gi...@apache.org>.
CRZbulabula commented on code in PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#discussion_r3200312108
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/impl/node/AddConfigNodeProcedure.java:
##########
@@ -80,9 +80,9 @@ protected Flow executeFromState(ConfigNodeProcedureEnv env, AddConfigNodeState s
LOG.info("Successfully ADD_PEER {}", tConfigNodeLocation);
break;
case REGISTER_SUCCESS:
- env.notifyRegisterSuccess(tConfigNodeLocation);
- env.createConfigNodeHeartbeatCache(tConfigNodeLocation.getConfigNodeId());
env.applyConfigNode(tConfigNodeLocation, versionInfo);
Review Comment:
**`applyConfigNode` can now throw `IllegalStateException`**
After the refactoring in `NodeManager.applyConfigNode()`, this call can throw `IllegalStateException` if the rollback after a version-write failure succeeds. The exception will be caught by the `catch (Exception e)` at line 89, which calls `setFailure(...)` only if `isRollbackSupported(state)` returns true for `REGISTER_SUCCESS`.
Please verify that `isRollbackSupported(AddConfigNodeState.REGISTER_SUCCESS)` returns `true`, otherwise the exception might be silently swallowed and the procedure could continue in an inconsistent state.
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#issuecomment-4386544769
## [](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17609) **Quality Gate passed**
Issues
 [7 New issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=17609&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=17609&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_iotdb&pullRequest=17609&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=17609&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=17609&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17609)
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "hongzhi-gao (via GitHub)" <gi...@apache.org>.
hongzhi-gao commented on PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#issuecomment-6208481098
1. [iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:628](https://github.com/apache/iotdb/blob/03b8d95a299c0ca082e73357b9633e121b289a27/iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java#L628)
- **Trigger:** The WAL write completes, but simpleLogWriter.force() throws.
- **Potential issue:** The catch returns failure without truncating the appended record or disabling further writes. write() then skips execution. A later successful force can persist that record, and startup replays it despite its never having been applied during normal operation. Subsequent plans can therefore execute against different metadata before and after restart. The captured pre-write offset should also protect persistence-failure paths.
2. [iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:670](https://github.com/apache/iotdb/blob/03b8d95a299c0ca082e73357b9633e121b289a27/iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java#L670)
- **Trigger:** Log rotation successfully closes the writer, then Files.move throws an IOException.
- **Potential issue:** closeSimpleLogWriter sets the writer to null before the rename. The exception returns through the persistence failure handler without reopening the original file. Every subsequent write fails the initialization check, so even a transient rename failure permanently prevents metadata writes until restart.
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "luoluoyuyu (via GitHub)" <gi...@apache.org>.
luoluoyuyu commented on code in PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#discussion_r3309337931
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:
##########
@@ -122,16 +122,32 @@ public TSStatus write(IConsensusRequest request) {
/** Transmit {@link ConfigPhysicalPlan} to {@link ConfigPlanExecutor} */
protected TSStatus write(ConfigPhysicalPlan plan) {
+ SimpleConsensusPersistResult persistResult = null;
+ if (ConsensusFactory.SIMPLE_CONSENSUS.equals(CONF.getConfigNodeConsensusProtocolClass())) {
+ persistResult = persistPlanForSimpleConsensus(plan);
Review Comment:
**WAL 顺序 persist → execute** 后，若 L127 `persistPlanForSimpleConsensus` 成功但 L136 `executeNonQueryPlan` 失败，plan 已落盘且**重启必重放**。
这要求**所有** `ConfigPhysicalPlan` 实现幂等。请提供：
1. 清单或审计结论（至少覆盖 RegisterDataNode、分区、pipe 相关 plan）
2. 单测/IT：模拟 execute 失败后重启，验证不 double-register、不抛不可恢复错误
否则 crash recovery 边缘场景可能损坏元数据。—— 与 @CRZbulabula 评论一致，请逐条回复。
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:
##########
@@ -122,16 +122,32 @@ public TSStatus write(IConsensusRequest request) {
/** Transmit {@link ConfigPhysicalPlan} to {@link ConfigPlanExecutor} */
protected TSStatus write(ConfigPhysicalPlan plan) {
+ SimpleConsensusPersistResult persistResult = null;
+ if (ConsensusFactory.SIMPLE_CONSENSUS.equals(CONF.getConfigNodeConsensusProtocolClass())) {
+ persistResult = persistPlanForSimpleConsensus(plan);
+ final TSStatus persistStatus = persistResult.status;
+ if (persistStatus.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
+ return persistStatus;
+ }
+ }
+
TSStatus result;
try {
result = executor.executeNonQueryPlan(plan);
Review Comment:
rollback 路径 `rollbackFailedPlanForSimpleConsensus` 在 execute 失败时触发，👍。
请确认 rollback 与 WAL 重放顺序在所有失败类型下一致，且 rollback 本身失败时 L145 的日志/告警足够让运维介入（避免 silent 元数据与 WAL 不一致）。
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "CRZbulabula (via GitHub)" <gi...@apache.org>.
CRZbulabula commented on code in PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#discussion_r3200312114
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/node/NodeManager.java:
##########
@@ -1303,6 +1322,144 @@ public TDataNodeLocation getLowestLoadDataNode(Set<Integer> nodes) {
return getRegisteredDataNode(dataNodeId).getLocation();
}
+ private TSStatus writeConfigPhysicalPlan(ConfigPhysicalPlan plan) {
+ try {
+ return getConsensusManager().write(plan);
+ } catch (ConsensusException e) {
+ LOGGER.warn(CONSENSUS_WRITE_ERROR, e);
+ return new TSStatus(TSStatusCode.EXECUTE_STATEMENT_ERROR.getStatusCode())
+ .setMessage(e.getMessage());
+ }
+ }
+
+ private boolean isConsensusWriteSuccessful(TSStatus status) {
+ return status != null && status.getCode() == TSStatusCode.SUCCESS_STATUS.getStatusCode();
+ }
+
+ private TSStatus rollbackDataNodeRegistration(
+ TDataNodeLocation dataNodeLocation, TSStatus versionUpdateStatus) {
+ final TSStatus rollbackStatus =
+ writeConfigPhysicalPlan(
+ new RemoveDataNodePlan(Collections.singletonList(dataNodeLocation)));
Review Comment:
**Rollback safety verified** — `RemoveDataNodePlan` (via `NodeInfo.removeDataNode()`) is a pure metadata removal from `registeredDataNodes` and `nodeVersionInfo` maps, with no side effects like region migration. Same for `RemoveAINodePlan` and `RemoveConfigNodePlan`. The rollback approach is safe.
One question: after a successful rollback, the `nodeId` that was generated via `nodeInfo.generateNextNodeId()` (line 326) is consumed but the node is removed. Does this leave a gap in the node ID sequence? If so, is that acceptable, or should `generateNextNodeId` be reverted on rollback to avoid ID fragmentation?
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "Caideyipi (via GitHub)" <gi...@apache.org>.
Caideyipi commented on PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#issuecomment-4608894210
Addressed the review feedback and pushed the updates.
Changes:
- Added WAL rollback/replay coverage: failed SimpleConsensus writes now assert that the rolled-back WAL contains no replayable plan.
- Added idempotency coverage for replay-relevant NodeInfo plans touched by this PR: RegisterDataNodePlan, ApplyConfigNodePlan, and UpdateVersionInfoPlan.
- Improved SimpleConsensus rollback failure logging with plan type, log file, truncate offset, and endIndexBeforeWrite; the returned status now also notes that the persisted plan may replay after restart if rollback fails.
- Removed newly introduced TODO comments; todo-check is now passing.
- Merged latest master and resolved the PartialPath conflict by keeping master's implementation, so the PR is mergeable again.
Verification:
- mvn -pl iotdb-core/confignode spotless:apply
- mvn -pl iotdb-core/node-commons spotless:apply
- mvn -pl iotdb-core/confignode '-Dtest=ConfigRegionStateMachineTest,NodeInfoTest' test
- git diff --check origin/master...HEAD
Current CI note: the Simple (17) failures were checkout/network failures on self-hosted runners (gnutls_handshake before tests started). I reran the failed jobs. todo-check has passed; the remaining jobs are still pending.
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#issuecomment-4386943917
## [Codecov](https://app.codecov.io/gh/apache/iotdb/pull/17609?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `32.36715%` with `140 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 40.09%. Comparing base ([`6042c4f`](https://app.codecov.io/gh/apache/iotdb/commit/6042c4f909f2be09fe2ce54412e6aafe1df747d7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`2047d3a`](https://app.codecov.io/gh/apache/iotdb/commit/2047d3a681b6a5713d96cfc84fc9952d3b212597?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 5 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/iotdb/pull/17609?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...che/iotdb/confignode/manager/node/NodeManager.java](https://app.codecov.io/gh/apache/iotdb/pull/17609?src=pr&el=tree&filepath=iotdb-core%2Fconfignode%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fiotdb%2Fconfignode%2Fmanager%2Fnode%2FNodeManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aW90ZGItY29yZS9jb25maWdub2RlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9pb3RkYi9jb25maWdub2RlL21hbmFnZXIvbm9kZS9Ob2RlTWFuYWdlci5qYXZh) | 42.30% | [75 Missing :warning: ](https://app.codecov.io/gh/apache/iotdb/pull/17609?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...nsensus/statemachine/ConfigRegionStateMachine.java](https://app.codecov.io/gh/apache/iotdb/pull/17609?src=pr&el=tree&filepath=iotdb-core%2Fconfignode%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fiotdb%2Fconfignode%2Fconsensus%2Fstatemachine%2FConfigRegionStateMachine.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aW90ZGItY29yZS9jb25maWdub2RlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9pb3RkYi9jb25maWdub2RlL2NvbnNlbnN1cy9zdGF0ZW1hY2hpbmUvQ29uZmlnUmVnaW9uU3RhdGVNYWNoaW5lLmphdmE=) | 9.23% | [59 Missing :warning: ](https://app.codecov.io/gh/apache/iotdb/pull/17609?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...gnode/persistence/executor/ConfigPlanExecutor.java](https://app.codecov.io/gh/apache/iotdb/pull/17609?src=pr&el=tree&filepath=iotdb-core%2Fconfignode%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fiotdb%2Fconfignode%2Fpersistence%2Fexecutor%2FConfigPlanExecutor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aW90ZGItY29yZS9jb25maWdub2RlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9pb3RkYi9jb25maWdub2RlL3BlcnNpc3RlbmNlL2V4ZWN1dG9yL0NvbmZpZ1BsYW5FeGVjdXRvci5qYXZh) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/iotdb/pull/17609?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...onfignode/persistence/partition/PartitionInfo.java](https://app.codecov.io/gh/apache/iotdb/pull/17609?src=pr&el=tree&filepath=iotdb-core%2Fconfignode%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fiotdb%2Fconfignode%2Fpersistence%2Fpartition%2FPartitionInfo.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aW90ZGItY29yZS9jb25maWdub2RlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9pb3RkYi9jb25maWdub2RlL3BlcnNpc3RlbmNlL3BhcnRpdGlvbi9QYXJ0aXRpb25JbmZvLmphdmE=) | 71.42% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/iotdb/pull/17609?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...de/procedure/impl/node/AddConfigNodeProcedure.java](https://app.codecov.io/gh/apache/iotdb/pull/17609?src=pr&el=tree&filepath=iotdb-core%2Fconfignode%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fiotdb%2Fconfignode%2Fprocedure%2Fimpl%2Fnode%2FAddConfigNodeProcedure.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aW90ZGItY29yZS9jb25maWdub2RlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9pb3RkYi9jb25maWdub2RlL3Byb2NlZHVyZS9pbXBsL25vZGUvQWRkQ29uZmlnTm9kZVByb2NlZHVyZS5qYXZh) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/iotdb/pull/17609?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17609 +/- ##
============================================
+ Coverage 39.94% 40.09% +0.14%
Complexity 2554 2554
============================================
Files 5176 5176
Lines 348546 348658 +112
Branches 44564 44586 +22
============================================
+ Hits 139242 139802 +560
+ Misses 209304 208856 -448
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/iotdb/pull/17609?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#issuecomment-4899959053
## [](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17609) **Quality Gate passed**
Issues
 [7 New issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=17609&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=17609&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_iotdb&pullRequest=17609&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=17609&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=17609&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17609)
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "Caideyipi (via GitHub)" <gi...@apache.org>.
Caideyipi commented on PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#issuecomment-6258683559
@hongzhi-gao Both issues are fixed in [1e1ffe04c8d](https://github.com/apache/iotdb/commit/1e1ffe04c8d035f46815ac91334a0d0183e9a913).
1. Append/force failure now rolls the WAL back to the saved pre-write offset and force-syncs that truncation. Cleanup failure disables further SimpleConsensus writes until restart. Regression coverage checks complete-record force failures, partial appends, preserved earlier records, and refusal of further writes when cleanup fails.
2. If rotation closes the writer but the atomic rename fails, it reopens the original log without advancing either index. `testRotationRenameFailureReopensOriginalLog` reproduces a real rename failure and verifies that the next write succeeds and both accepted plans can be replayed. A failed reopen explicitly disables further writes.
Validation: full-reactor test compilation in English and Chinese; 12 focused tests passed in each locale; ConfigNode Spotless and Checkstyle passed; 431 locale keys and format placeholders match.
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "CRZbulabula (via GitHub)" <gi...@apache.org>.
CRZbulabula commented on code in PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#discussion_r3200611468
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/persistence/partition/DatabasePartitionTable.java:
##########
@@ -243,7 +243,7 @@ public int getRegionGroupCount(TConsensusGroupType type) {
result.getAndIncrement();
}
});
- return result.getAndIncrement();
+ return result.get();
Review Comment:
Is this a bug?
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#issuecomment-4608897478
## [](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17609) **Quality Gate passed**
Issues
 [6 New issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=17609&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=17609&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_iotdb&pullRequest=17609&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=17609&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=17609&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17609)
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "CRZbulabula (via GitHub)" <gi...@apache.org>.
CRZbulabula commented on code in PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#discussion_r4291714840
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:
##########
@@ -579,60 +602,84 @@ public boolean isReadOnly() {
return CommonDescriptor.getInstance().getConfig().isReadOnly();
}
- private void writeLogForSimpleConsensus(ConfigPhysicalPlan plan) {
- if (simpleLogFile.length() > LOG_FILE_MAX_SIZE) {
- try {
- simpleLogWriter.force();
- File completedFilePath = new File(FILE_PATH + startIndex + "_" + endIndex);
- Files.move(
- simpleLogFile.toPath(), completedFilePath.toPath(), StandardCopyOption.ATOMIC_MOVE);
- } catch (IOException e) {
- LOGGER.error(
- ConfigNodeMessages.CAN_T_FORCE_LOGWRITER_FOR_CONFIGNODE_SIMPLECONSENSUS_MODE, e);
+ private SimpleConsensusPersistResult persistPlanForSimpleConsensus(ConfigPhysicalPlan plan) {
+ File persistedLogFile = null;
+ long logFileSizeBeforeWrite = 0;
+ int endIndexBeforeWrite = endIndex;
+ try {
+ if (simpleLogWriter == null || simpleLogFile == null) {
+ throw new IOException(ConfigNodeMessages.SIMPLECONSENSUS_LOG_WRITER_IS_NOT_INITIALIZED);
}
- for (int retry = 0; retry < 5; retry++) {
- try {
- simpleLogWriter.close();
- } catch (IOException e) {
- LOGGER.warn(
- ConfigNodeMessages
- .LOG_CONFIGREGION_UPDATE_FAILED_TO_CLOSE_THE_CONFIGNODE_SIMPLECONSENSUS_LOG_FILEPATH_ARG_RETRY_ARG_188A702E,
- simpleLogFile.getAbsolutePath(),
- retry);
- try {
- // Sleep 1s and retry
- TimeUnit.SECONDS.sleep(1);
- } catch (InterruptedException e2) {
- Thread.currentThread().interrupt();
- LOGGER.warn(
- ConfigNodeMessages.UNEXPECTED_INTERRUPTION_DURING_THE_CLOSE_METHOD_OF_LOGWRITER);
- }
- continue;
- }
- break;
+
+ if (simpleLogFile.length() > LOG_FILE_MAX_SIZE) {
+ rollSimpleConsensusLogFile();
}
- startIndex = endIndex + 1;
- createLogFile(startIndex);
- }
- try {
+ persistedLogFile = simpleLogFile;
+ logFileSizeBeforeWrite = persistedLogFile.length();
+ endIndexBeforeWrite = endIndex;
+
ByteBuffer buffer = plan.serializeToByteBuffer();
buffer.position(buffer.limit());
simpleLogWriter.write(buffer);
+ simpleLogWriter.force();
endIndex = endIndex + 1;
} catch (Exception e) {
LOGGER.error(
ConfigNodeMessages
- .CAN_T_SERIALIZE_CURRENT_CONFIGPHYSICALPLAN_FOR_CONFIGNODE_SIMPLECONSENSUS_MODE,
+ .PERSIST_CURRENT_CONFIGPHYSICALPLAN_FOR_CONFIGNODE_SIMPLECONSENSUS_MODE_FAILED,
e);
+ return SimpleConsensusPersistResult.failure(
Review Comment:
**[P1] Remove the appended WAL record when persistence fails**
- If `simpleLogWriter.write(buffer)` succeeds but `force()` throws, this catch returns a failure without truncating the record already appended to the WAL.
- `write()` then returns before calling the executor, but restart recovery replays the record, so an operation rejected to the caller takes effect after restart. A partial append can also leave an invalid record ahead of subsequent writes.
- I reproduced the complete-record case with an injected `force()` failure: status `301`, zero executor calls, and a 36-byte WAL record that `SingleFileLogReader` successfully replays as `UpdateVersionInfo`.
Please restore and force-sync the saved pre-write offset on persistence failure, and prevent further writes if that cleanup cannot complete. Add coverage for append/force failures as well as executor failures.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/node/NodeManager.java:
##########
@@ -1385,6 +1412,144 @@ public TDataNodeLocation getLowestLoadDataNode(Set<Integer> nodes) {
return getRegisteredDataNode(dataNodeId).getLocation();
}
+ private TSStatus writeConfigPhysicalPlan(ConfigPhysicalPlan plan) {
+ try {
+ return getConsensusManager().write(plan);
+ } catch (ConsensusException e) {
+ LOGGER.warn(CONSENSUS_WRITE_ERROR, e);
+ return new TSStatus(TSStatusCode.EXECUTE_STATEMENT_ERROR.getStatusCode())
+ .setMessage(e.getMessage());
+ }
+ }
+
+ private boolean isConsensusWriteSuccessful(TSStatus status) {
+ return status != null && status.getCode() == TSStatusCode.SUCCESS_STATUS.getStatusCode();
+ }
+
+ private TSStatus rollbackDataNodeRegistration(
+ TDataNodeLocation dataNodeLocation, TSStatus versionUpdateStatus) {
+ final TSStatus rollbackStatus =
+ writeConfigPhysicalPlan(
+ new RemoveDataNodePlan(Collections.singletonList(dataNodeLocation)));
+ final String failureMessage =
+ String.format(
+ "Failed to persist version info for DataNode %d: %s",
+ dataNodeLocation.getDataNodeId(), describeStatus(versionUpdateStatus));
+ if (isConsensusWriteSuccessful(rollbackStatus)) {
+ return buildStatus(
+ versionUpdateStatus.getCode(),
+ failureMessage,
+ "The registration has been rolled back. Please retry the registration.");
+ }
+
+ LOGGER.error(
+ "Failed to roll back DataNode registration {} after version info persistence failure. "
+ + ROLLBACK_STATUS_LOG_FORMAT,
+ dataNodeLocation,
+ versionUpdateStatus,
+ rollbackStatus);
+ return buildStatus(
+ rollbackStatus.getCode(),
+ failureMessage,
+ String.format(REGISTRATION_ROLLBACK_FAILED_FORMAT, describeStatus(rollbackStatus)),
+ MANUAL_REGISTRATION_CLEANUP_MESSAGE);
+ }
+
+ private TSStatus rollbackAiNodeRegistration(
+ TAINodeLocation aiNodeLocation, TSStatus versionUpdateStatus) {
+ final TSStatus rollbackStatus = writeConfigPhysicalPlan(new RemoveAINodePlan(aiNodeLocation));
+ final String failureMessage =
+ String.format(
+ "Failed to persist version info for AINode %d: %s",
+ aiNodeLocation.getAiNodeId(), describeStatus(versionUpdateStatus));
+ if (isConsensusWriteSuccessful(rollbackStatus)) {
+ return buildStatus(
+ versionUpdateStatus.getCode(),
+ failureMessage,
+ "The registration has been rolled back. Please retry the registration.");
+ }
+
+ LOGGER.error(
+ "Failed to roll back AINode registration {} after version info persistence failure. "
+ + ROLLBACK_STATUS_LOG_FORMAT,
+ aiNodeLocation,
+ versionUpdateStatus,
+ rollbackStatus);
+ return buildStatus(
+ rollbackStatus.getCode(),
+ failureMessage,
+ String.format(REGISTRATION_ROLLBACK_FAILED_FORMAT, describeStatus(rollbackStatus)),
+ MANUAL_REGISTRATION_CLEANUP_MESSAGE);
+ }
+
+ private TSStatus rollbackConfigNodeRegistration(
+ TConfigNodeLocation configNodeLocation, TSStatus versionUpdateStatus) {
+ final TSStatus rollbackStatus =
+ writeConfigPhysicalPlan(new RemoveConfigNodePlan(configNodeLocation));
Review Comment:
**[P1] Avoid decommissioning the joining ConfigNode during registration rollback**
- At this point the joining ConfigNode is already a Raft member: `AddConfigNodeProcedure` completes `ADD_PEER` before entering `REGISTER_SUCCESS`. It therefore also applies this `RemoveConfigNodePlan`.
- `NodeInfo.removeConfigNode()` sets the local node status to `Removing` when the removed ID is its own. The procedure retries `REGISTER_SUCCESS` after this exception, but neither `applyConfigNode()` nor `notifyRegisterSuccess()` resets that status.
- A transient version-write failure followed by a successful retry consequently leaves the newly registered ConfigNode rejecting requests in `ConfigManager.confirmLeader()`.
Please use a registration rollback that avoids node-removal lifecycle side effects, or explicitly restore the appropriate node status when registration succeeds. A regression test should cover version-write failure followed by a successful registration retry.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/node/NodeManager.java:
##########
@@ -1385,6 +1412,144 @@ public TDataNodeLocation getLowestLoadDataNode(Set<Integer> nodes) {
return getRegisteredDataNode(dataNodeId).getLocation();
}
+ private TSStatus writeConfigPhysicalPlan(ConfigPhysicalPlan plan) {
+ try {
+ return getConsensusManager().write(plan);
+ } catch (ConsensusException e) {
+ LOGGER.warn(CONSENSUS_WRITE_ERROR, e);
+ return new TSStatus(TSStatusCode.EXECUTE_STATEMENT_ERROR.getStatusCode())
+ .setMessage(e.getMessage());
+ }
+ }
+
+ private boolean isConsensusWriteSuccessful(TSStatus status) {
+ return status != null && status.getCode() == TSStatusCode.SUCCESS_STATUS.getStatusCode();
+ }
+
+ private TSStatus rollbackDataNodeRegistration(
+ TDataNodeLocation dataNodeLocation, TSStatus versionUpdateStatus) {
+ final TSStatus rollbackStatus =
+ writeConfigPhysicalPlan(
+ new RemoveDataNodePlan(Collections.singletonList(dataNodeLocation)));
+ final String failureMessage =
+ String.format(
+ "Failed to persist version info for DataNode %d: %s",
+ dataNodeLocation.getDataNodeId(), describeStatus(versionUpdateStatus));
Review Comment:
**[P2] Localize the new registration failure templates**
- This template, the rollback result messages/logs below, and the analogous AINode/ConfigNode helpers introduce raw English strings in Java code. The new exception/status descriptions and the constants near the top of this class have the same issue.
- These strings remain English under `-P with-zh-locale`, because that build only swaps the locale-specific message classes.
Please put each complete user-visible or operator-visible template in both the English and Chinese `ConfigNodeMessages` classes, preserve its format placeholders, and reference those constants here. This also applies to the new rollback log and exception templates.
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "CRZbulabula (via GitHub)" <gi...@apache.org>.
CRZbulabula commented on code in PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#discussion_r3200312085
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:
##########
@@ -121,6 +121,13 @@ public TSStatus write(IConsensusRequest request) {
/** Transmit {@link ConfigPhysicalPlan} to {@link ConfigPlanExecutor} */
protected TSStatus write(ConfigPhysicalPlan plan) {
+ if (ConsensusFactory.SIMPLE_CONSENSUS.equals(CONF.getConfigNodeConsensusProtocolClass())) {
+ final TSStatus persistStatus = persistPlanForSimpleConsensus(plan);
+ if (persistStatus.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
+ return persistStatus;
+ }
+ }
Review Comment:
**WAL replay idempotency requirement**
With the new write-ahead ordering (persist → execute), if `persistPlanForSimpleConsensus` succeeds at line 125 but `executor.executeNonQueryPlan` fails at line 133, the plan is already persisted in WAL and **will be replayed on restart**.
This is correct WAL semantics, but it introduces a hard requirement: **all `ConfigPhysicalPlan` implementations must be idempotent under replay.** For example, a `RegisterDataNodePlan` replayed against an already-registered node must not fail or double-register.
Is this idempotency property currently guaranteed across all ConfigPhysicalPlan types? If not, this could cause issues on crash recovery in edge cases.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:
##########
@@ -351,56 +360,48 @@ public boolean isReadOnly() {
return CommonDescriptor.getInstance().getConfig().isReadOnly();
}
- private void writeLogForSimpleConsensus(ConfigPhysicalPlan plan) {
- if (simpleLogFile.length() > LOG_FILE_MAX_SIZE) {
- try {
- simpleLogWriter.force();
- File completedFilePath = new File(FILE_PATH + startIndex + "_" + endIndex);
- Files.move(
- simpleLogFile.toPath(), completedFilePath.toPath(), StandardCopyOption.ATOMIC_MOVE);
- } catch (IOException e) {
- LOGGER.error("Can't force logWriter for ConfigNode SimpleConsensus mode", e);
+ private TSStatus persistPlanForSimpleConsensus(ConfigPhysicalPlan plan) {
+ try {
+ if (simpleLogWriter == null || simpleLogFile == null) {
+ throw new IOException("SimpleConsensus log writer is not initialized.");
}
- for (int retry = 0; retry < 5; retry++) {
- try {
- simpleLogWriter.close();
- } catch (IOException e) {
- LOGGER.warn(
- "Can't close StandAloneLog for ConfigNode SimpleConsensus mode, "
- + "filePath: {}, retry: {}",
- simpleLogFile.getAbsolutePath(),
- retry);
- try {
- // Sleep 1s and retry
- TimeUnit.SECONDS.sleep(1);
- } catch (InterruptedException e2) {
- Thread.currentThread().interrupt();
- LOGGER.warn("Unexpected interruption during the close method of logWriter");
- }
- continue;
- }
- break;
+
+ if (simpleLogFile.length() > LOG_FILE_MAX_SIZE) {
+ rollSimpleConsensusLogFile();
}
- startIndex = endIndex + 1;
- createLogFile(startIndex);
- }
- try {
ByteBuffer buffer = plan.serializeToByteBuffer();
buffer.position(buffer.limit());
simpleLogWriter.write(buffer);
+ simpleLogWriter.force();
Review Comment:
**`force()` on every write makes scheduled flush redundant**
Now that every write is synchronously forced here, the scheduled `flushWALForSimpleConsensus` thread (lines 447-455) becomes redundant — it calls `simpleLogWriter.force()` periodically, but there's nothing left to flush.
This is harmless (double-force is idempotent) but worth either:
1. Removing the scheduled thread to avoid confusion, or
2. Adding a brief comment explaining the intentional belt-and-suspenders approach.
For ConfigNode metadata operations (low write frequency), the per-write `force()` is the right durability choice.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:
##########
@@ -351,56 +360,48 @@ public boolean isReadOnly() {
return CommonDescriptor.getInstance().getConfig().isReadOnly();
}
- private void writeLogForSimpleConsensus(ConfigPhysicalPlan plan) {
- if (simpleLogFile.length() > LOG_FILE_MAX_SIZE) {
- try {
- simpleLogWriter.force();
- File completedFilePath = new File(FILE_PATH + startIndex + "_" + endIndex);
- Files.move(
- simpleLogFile.toPath(), completedFilePath.toPath(), StandardCopyOption.ATOMIC_MOVE);
- } catch (IOException e) {
- LOGGER.error("Can't force logWriter for ConfigNode SimpleConsensus mode", e);
+ private TSStatus persistPlanForSimpleConsensus(ConfigPhysicalPlan plan) {
+ try {
+ if (simpleLogWriter == null || simpleLogFile == null) {
+ throw new IOException("SimpleConsensus log writer is not initialized.");
}
- for (int retry = 0; retry < 5; retry++) {
- try {
- simpleLogWriter.close();
- } catch (IOException e) {
- LOGGER.warn(
- "Can't close StandAloneLog for ConfigNode SimpleConsensus mode, "
- + "filePath: {}, retry: {}",
- simpleLogFile.getAbsolutePath(),
- retry);
- try {
- // Sleep 1s and retry
- TimeUnit.SECONDS.sleep(1);
- } catch (InterruptedException e2) {
- Thread.currentThread().interrupt();
- LOGGER.warn("Unexpected interruption during the close method of logWriter");
- }
- continue;
- }
- break;
+
+ if (simpleLogFile.length() > LOG_FILE_MAX_SIZE) {
+ rollSimpleConsensusLogFile();
}
- startIndex = endIndex + 1;
- createLogFile(startIndex);
- }
- try {
ByteBuffer buffer = plan.serializeToByteBuffer();
buffer.position(buffer.limit());
simpleLogWriter.write(buffer);
+ simpleLogWriter.force();
endIndex = endIndex + 1;
} catch (Exception e) {
LOGGER.error(
- "Can't serialize current ConfigPhysicalPlan for ConfigNode SimpleConsensus mode", e);
+ "Persist current ConfigPhysicalPlan for ConfigNode SimpleConsensus mode failed", e);
+ return new TSStatus(TSStatusCode.EXECUTE_STATEMENT_ERROR.getStatusCode())
+ .setMessage(
+ "Persist ConfigNode SimpleConsensus log failed: " + String.valueOf(e.getMessage()));
Review Comment:
Nit: `String.valueOf(e.getMessage())` is redundant — string concatenation with `+` already handles `null` by converting to `"null"`. Can simplify to:
```java
"Persist ConfigNode SimpleConsensus log failed: " + e.getMessage()
```
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/node/NodeManager.java:
##########
@@ -1303,6 +1322,144 @@ public TDataNodeLocation getLowestLoadDataNode(Set<Integer> nodes) {
return getRegisteredDataNode(dataNodeId).getLocation();
}
+ private TSStatus writeConfigPhysicalPlan(ConfigPhysicalPlan plan) {
+ try {
+ return getConsensusManager().write(plan);
+ } catch (ConsensusException e) {
+ LOGGER.warn(CONSENSUS_WRITE_ERROR, e);
+ return new TSStatus(TSStatusCode.EXECUTE_STATEMENT_ERROR.getStatusCode())
+ .setMessage(e.getMessage());
+ }
+ }
+
+ private boolean isConsensusWriteSuccessful(TSStatus status) {
+ return status != null && status.getCode() == TSStatusCode.SUCCESS_STATUS.getStatusCode();
+ }
+
+ private TSStatus rollbackDataNodeRegistration(
+ TDataNodeLocation dataNodeLocation, TSStatus versionUpdateStatus) {
+ final TSStatus rollbackStatus =
+ writeConfigPhysicalPlan(
+ new RemoveDataNodePlan(Collections.singletonList(dataNodeLocation)));
Review Comment:
**Rollback safety verified** — `RemoveDataNodePlan` (via `NodeInfo.removeDataNode()`) is a pure metadata removal from `registeredDataNodes` and `nodeVersionInfo` maps, with no side effects like region migration. Same for `RemoveAINodePlan` and `RemoveConfigNodePlan`. The rollback approach is safe.
One question: after a successful rollback, the `nodeId` that was generated via `nodeInfo.generateNextNodeId()` (line 326) is consumed but the node is removed. Does this leave a gap in the node ID sequence? If so, is that acceptable, or should `generateNextNodeId` be reverted on rollback to avoid ID fragmentation?
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/impl/node/AddConfigNodeProcedure.java:
##########
@@ -80,9 +80,9 @@ protected Flow executeFromState(ConfigNodeProcedureEnv env, AddConfigNodeState s
LOG.info("Successfully ADD_PEER {}", tConfigNodeLocation);
break;
case REGISTER_SUCCESS:
- env.notifyRegisterSuccess(tConfigNodeLocation);
- env.createConfigNodeHeartbeatCache(tConfigNodeLocation.getConfigNodeId());
env.applyConfigNode(tConfigNodeLocation, versionInfo);
Review Comment:
**`applyConfigNode` can now throw `IllegalStateException`**
After the refactoring in `NodeManager.applyConfigNode()`, this call can throw `IllegalStateException` if the rollback after a version-write failure succeeds. The exception will be caught by the `catch (Exception e)` at line 89, which calls `setFailure(...)` only if `isRollbackSupported(state)` returns true for `REGISTER_SUCCESS`.
Please verify that `isRollbackSupported(AddConfigNodeState.REGISTER_SUCCESS)` returns `true`, otherwise the exception might be silently swallowed and the procedure could continue in an inconsistent state.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/persistence/partition/PartitionInfo.java:
##########
@@ -1008,9 +1010,14 @@ public boolean processTakeSnapshot(File snapshotDir) throws TException, IOExcept
databasePartitionTableEntry.getValue().serialize(bufferedOutputStream, protocol);
}
+ final List<RegionMaintainTask> copiedRegionMaintainTaskList;
+ synchronized (regionMaintainTaskList) {
+ copiedRegionMaintainTaskList = new ArrayList<>(regionMaintainTaskList);
+ }
Review Comment:
Good fix — `synchronized` copy before iterating. One minor observation: this copies the entire list under the lock. If `regionMaintainTaskList` can grow very large, this could cause a brief pause. In practice, for ConfigNode this list is typically small, so this is fine. Just noting for future awareness.
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "luoluoyuyu (via GitHub)" <gi...@apache.org>.
luoluoyuyu commented on code in PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#discussion_r3309337931
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:
##########
@@ -122,16 +122,32 @@ public TSStatus write(IConsensusRequest request) {
/** Transmit {@link ConfigPhysicalPlan} to {@link ConfigPlanExecutor} */
protected TSStatus write(ConfigPhysicalPlan plan) {
+ SimpleConsensusPersistResult persistResult = null;
+ if (ConsensusFactory.SIMPLE_CONSENSUS.equals(CONF.getConfigNodeConsensusProtocolClass())) {
+ persistResult = persistPlanForSimpleConsensus(plan);
Review Comment:
**WAL 顺序 persist → execute** 后，若 L127 `persistPlanForSimpleConsensus` 成功但 L136 `executeNonQueryPlan` 失败，plan 已落盘且**重启必重放**。
这要求**所有** `ConfigPhysicalPlan` 实现幂等。请提供：
1. 清单或审计结论（至少覆盖 RegisterDataNode、分区、pipe 相关 plan）
2. 单测/IT：模拟 execute 失败后重启，验证不 double-register、不抛不可恢复错误
否则 crash recovery 边缘场景可能损坏元数据。—— 与 @CRZbulabula 评论一致，请逐条回复。
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:
##########
@@ -122,16 +122,32 @@ public TSStatus write(IConsensusRequest request) {
/** Transmit {@link ConfigPhysicalPlan} to {@link ConfigPlanExecutor} */
protected TSStatus write(ConfigPhysicalPlan plan) {
+ SimpleConsensusPersistResult persistResult = null;
+ if (ConsensusFactory.SIMPLE_CONSENSUS.equals(CONF.getConfigNodeConsensusProtocolClass())) {
+ persistResult = persistPlanForSimpleConsensus(plan);
+ final TSStatus persistStatus = persistResult.status;
+ if (persistStatus.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
+ return persistStatus;
+ }
+ }
+
TSStatus result;
try {
result = executor.executeNonQueryPlan(plan);
Review Comment:
rollback 路径 `rollbackFailedPlanForSimpleConsensus` 在 execute 失败时触发，👍。
请确认 rollback 与 WAL 重放顺序在所有失败类型下一致，且 rollback 本身失败时 L145 的日志/告警足够让运维介入（避免 silent 元数据与 WAL 不一致）。
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "luoluoyuyu (via GitHub)" <gi...@apache.org>.
luoluoyuyu commented on code in PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#discussion_r3309372352
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:
##########
@@ -122,16 +122,32 @@ public TSStatus write(IConsensusRequest request) {
/** Transmit {@link ConfigPhysicalPlan} to {@link ConfigPlanExecutor} */
protected TSStatus write(ConfigPhysicalPlan plan) {
+ SimpleConsensusPersistResult persistResult = null;
+ if (ConsensusFactory.SIMPLE_CONSENSUS.equals(CONF.getConfigNodeConsensusProtocolClass())) {
+ persistResult = persistPlanForSimpleConsensus(plan);
+ final TSStatus persistStatus = persistResult.status;
+ if (persistStatus.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
+ return persistStatus;
+ }
+ }
+
TSStatus result;
try {
result = executor.executeNonQueryPlan(plan);
Review Comment:
rollbackFailedPlanForSimpleConsensus on execute failure is the right safety net. Ensure rollback failure paths are logged clearly enough for operators when WAL and in-memory state may diverge.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:
##########
@@ -122,16 +122,32 @@ public TSStatus write(IConsensusRequest request) {
/** Transmit {@link ConfigPhysicalPlan} to {@link ConfigPlanExecutor} */
protected TSStatus write(ConfigPhysicalPlan plan) {
+ SimpleConsensusPersistResult persistResult = null;
+ if (ConsensusFactory.SIMPLE_CONSENSUS.equals(CONF.getConfigNodeConsensusProtocolClass())) {
+ persistResult = persistPlanForSimpleConsensus(plan);
Review Comment:
With persist before execute, a successful persistPlanForSimpleConsensus followed by a failed executeNonQueryPlan leaves the plan in WAL and it will be replayed on restart. Every ConfigPhysicalPlan executed on replay must be idempotent. Please add an audit or tests for the plan types touched by this PR.
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "Caideyipi (via GitHub)" <gi...@apache.org>.
Caideyipi commented on PR #17609:
URL: https://github.com/apache/iotdb/pull/17609#issuecomment-5127855946
Verified that the review changes already present in `cddb4b1c962` remain intact after the latest master merge, and replied to every inline thread without resolving them.
Current verification:
- `ConfigRegionStateMachineTest`: 4 passed
- `NodeInfoTest`: 2 passed
- `PartitionInfoTest`: 5 passed
The current Windows DataNode UT failure is unrelated to this PR: the runner hit Java heap OOM, followed by a timing-sensitive `L1PriorityQueueTest.testPollBlocked` failure; 4,420 tests ran and the PR changes do not touch that code. The Sonar Reliability-C issues are also reported in files outside this PR's 11-file diff after the master merge, so there is no additional #17609 code change to apply for those checks.
--
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] Fixed multiple configNode bugs [iotdb]
Posted by "Caideyipi (via GitHub)" <gi...@apache.org>.
Caideyipi merged PR #17609:
URL: https://github.com/apache/iotdb/pull/17609
--
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