Repository navigation
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #17609 +/- ##
============================================
- Coverage 43.40% 43.33% -0.08%
Complexity 374 374
============================================
Files 5366 5364 -2
Lines 382885 383319 +434
Branches 49796 49880 +84
============================================
- Hits 166187 166104 -83
- Misses 216698 217215 +517 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
CRZbulabula
left a comment
CRZbulabula
left a comment
There was a problem hiding this comment.
Overall
This PR fixes 6 real correctness bugs in ConfigNode (WAL ordering, log sorting, unchecked consensus writes, off-by-one, concurrent snapshot, pipe-listener failure masking). Changes are well-structured, rollback logic is carefully designed, and test coverage for core scenarios is solid.
Inline comments below cover a few items worth attention before merge.
| if (persistStatus.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()) { | ||
| return persistStatus; | ||
| } | ||
| } |
There was a problem hiding this 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.
There was a problem hiding this comment.
Addressed in cddb4b1. 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.
| ByteBuffer buffer = plan.serializeToByteBuffer(); | ||
| buffer.position(buffer.limit()); | ||
| simpleLogWriter.write(buffer); | ||
| simpleLogWriter.force(); |
There was a problem hiding this 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:
- Removing the scheduled thread to avoid confusion, or
- 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.
There was a problem hiding this 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().
| "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())); |
There was a problem hiding this comment.
Nit: String.valueOf(e.getMessage()) is redundant — string concatenation with + already handles null by converting to "null". Can simplify to:
"Persist ConfigNode SimpleConsensus log failed: " + e.getMessage()There was a problem hiding this comment.
Addressed in the rewritten persistence error path: the message now directly concatenates e.getMessage().
| final List<RegionMaintainTask> copiedRegionMaintainTaskList; | ||
| synchronized (regionMaintainTaskList) { | ||
| copiedRegionMaintainTaskList = new ArrayList<>(regionMaintainTaskList); | ||
| } |
There was a problem hiding this 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.
There was a problem hiding this 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.
| } | ||
| }); | ||
| return result.getAndIncrement(); | ||
| return result.get(); |
There was a problem hiding this 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.
# Conflicts: # iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java # iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/impl/node/AddConfigNodeProcedure.java
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
luoluoyuyu
left a comment
luoluoyuyu
left a comment
There was a problem hiding this comment.
Review summary
Fixes several ConfigNode correctness issues including WAL persist-before-execute and rollback on execute failure. Before merge, document or test that all ConfigPhysicalPlan types are idempotent under WAL replay when persist succeeds and execute fails.
Also resolve CI failures in this PR.
| protected TSStatus write(ConfigPhysicalPlan plan) { | ||
| SimpleConsensusPersistResult persistResult = null; | ||
| if (ConsensusFactory.SIMPLE_CONSENSUS.equals(CONF.getConfigNodeConsensusProtocolClass())) { | ||
| persistResult = persistPlanForSimpleConsensus(plan); |
There was a problem hiding this 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.
There was a problem hiding this 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.
|
|
||
| TSStatus result; | ||
| try { | ||
| result = executor.executeNonQueryPlan(plan); |
There was a problem hiding this 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.
There was a problem hiding this 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.
# Conflicts: # iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/path/PartialPath.java
|
Addressed the review feedback and pushed the updates. Changes:
Verification:
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. |
|
# Conflicts: # iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java # iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/persistence/executor/ConfigPlanExecutor.java
|
Verified that the review changes already present in Current verification:
The current Windows DataNode UT failure is unrelated to this PR: the runner hit Java heap OOM, followed by a timing-sensitive |
|
|
CRZbulabula
left a comment
CRZbulabula
left a comment
There was a problem hiding this comment.
Three findings from reviewing the current PR head: WAL persistence failure cleanup, ConfigNode registration rollback side effects, and localization of the new registration errors. Details are attached inline.
| .CAN_T_SERIALIZE_CURRENT_CONFIGPHYSICALPLAN_FOR_CONFIGNODE_SIMPLECONSENSUS_MODE, | ||
| .PERSIST_CURRENT_CONFIGPHYSICALPLAN_FOR_CONFIGNODE_SIMPLECONSENSUS_MODE_FAILED, | ||
| e); | ||
| return SimpleConsensusPersistResult.failure( |
There was a problem hiding this comment.
[P1] Remove the appended WAL record when persistence fails
- If
simpleLogWriter.write(buffer)succeeds butforce()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: status301, zero executor calls, and a 36-byte WAL record thatSingleFileLogReadersuccessfully replays asUpdateVersionInfo.
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.
| private TSStatus rollbackConfigNodeRegistration( | ||
| TConfigNodeLocation configNodeLocation, TSStatus versionUpdateStatus) { | ||
| final TSStatus rollbackStatus = | ||
| writeConfigPhysicalPlan(new RemoveConfigNodePlan(configNodeLocation)); |
There was a problem hiding this comment.
[P1] Avoid decommissioning the joining ConfigNode during registration rollback
- At this point the joining ConfigNode is already a Raft member:
AddConfigNodeProcedurecompletesADD_PEERbefore enteringREGISTER_SUCCESS. It therefore also applies thisRemoveConfigNodePlan. NodeInfo.removeConfigNode()sets the local node status toRemovingwhen the removed ID is its own. The procedure retriesREGISTER_SUCCESSafter this exception, but neitherapplyConfigNode()nornotifyRegisterSuccess()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.
| final String failureMessage = | ||
| String.format( | ||
| "Failed to persist version info for DataNode %d: %s", | ||
| dataNodeLocation.getDataNodeId(), describeStatus(versionUpdateStatus)); |
There was a problem hiding this 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.
4 participants
Description
This PR has:
for an unfamiliar reader.
for code coverage.
Key changed/added classes (or packages if there are too many classes) in this PR