-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix folderManager NPE issue in WALNode when starting iotdb in a disk-full state #16869
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: OneSizeFitsQuorum <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses an NPE issue in WALNode creation that occurs when IoTDB starts with a full disk. When the disk is full at startup, the FolderManager initialization in the constructor fails, leaving it null. If disk space is later freed, subsequent WAL node creation attempts fail with NPE because the folderManager is still null.
Key Changes:
- Adds lazy initialization of FolderManager in the createWALNode method to handle recovery after disk space becomes available
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.../apache/iotdb/db/storageengine/dataregion/wal/allocation/AbstractNodeAllocationStrategy.java
Show resolved
Hide resolved
Signed-off-by: OneSizeFitsQuorum <[email protected]>
ca51b0d to
cd69d06
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #16869 +/- ##
============================================
+ Coverage 38.87% 39.05% +0.17%
Complexity 207 207
============================================
Files 5021 5021
Lines 333052 333150 +98
Branches 42389 42390 +1
============================================
+ Hits 129487 130119 +632
+ Misses 203565 203031 -534 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.../apache/iotdb/db/storageengine/dataregion/wal/allocation/AbstractNodeAllocationStrategy.java
Outdated
Show resolved
Hide resolved
.../apache/iotdb/db/storageengine/dataregion/wal/allocation/AbstractNodeAllocationStrategy.java
Outdated
Show resolved
Hide resolved
.../apache/iotdb/db/storageengine/dataregion/wal/allocation/AbstractNodeAllocationStrategy.java
Outdated
Show resolved
Hide resolved
Signed-off-by: OneSizeFitsQuorum <[email protected]>
51b2f9a to
fdf0e8f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ava/org/apache/iotdb/db/storageengine/dataregion/wal/allocation/FirstCreateStrategyTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: OneSizeFitsQuorum <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ava/org/apache/iotdb/db/storageengine/dataregion/wal/allocation/FirstCreateStrategyTest.java
Show resolved
Hide resolved
...ava/org/apache/iotdb/db/storageengine/dataregion/wal/allocation/FirstCreateStrategyTest.java
Show resolved
Hide resolved
...ava/org/apache/iotdb/db/storageengine/dataregion/wal/allocation/FirstCreateStrategyTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: OneSizeFitsQuorum <[email protected]>
Signed-off-by: OneSizeFitsQuorum <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
iotdb-core/datanode/src/test/java/org/apache/iotdb/db/storageengine/dataregion/wal/allocation/FirstCreateStrategyTest.java:256
- Using reflection to access internal state for assertions makes the test brittle and tightly coupled to implementation details. If the field name changes or access control is modified, the test will break. Consider adding a package-private getter method or test hook in the production code to support this test scenario without reflection.
new MeasurementSchema("s2", dataTypes[1]),
new MeasurementSchema("s3", dataTypes[2]),
new MeasurementSchema("s4", dataTypes[3]),
new MeasurementSchema("s5", dataTypes[4]),
new MeasurementSchema("s6", dataTypes[5]),
};
return new InsertRowNode(
new PlanNodeId("0"),
iotdb-core/datanode/src/test/java/org/apache/iotdb/db/storageengine/dataregion/wal/allocation/FirstCreateStrategyTest.java:228
- The test name and comment suggest testing behavior when disk is still full, but the test uses non-existent directories instead. Non-existent directories may not accurately simulate a disk-full scenario. The FolderManager constructor might fail for different reasons (directory doesn't exist vs. disk full), leading to different error paths. Consider creating actual directories and simulating disk-full conditions more accurately, or update the test name and comments to reflect what is actually being tested.
new TSDataType[] {
iotdb-core/datanode/src/test/java/org/apache/iotdb/db/storageengine/dataregion/wal/allocation/FirstCreateStrategyTest.java:271
- The assertion checks if fakeNode.toString() contains "DiskSpaceInsufficientException" or "Failed to create WAL node", but WALFakeNode does not override toString(). This will use the default Object.toString() which returns the class name and hashcode, never matching these strings. The assertion will always fail. Consider checking the failure listener's exception message instead, or add a getter method to WALFakeNode to expose the failure cause.
iotdb-core/datanode/src/test/java/org/apache/iotdb/db/storageengine/dataregion/wal/allocation/FirstCreateStrategyTest.java:270 - Default toString(): WALFakeNode inherits toString() from Object, and so is not suitable for printing.
iotdb-core/datanode/src/test/java/org/apache/iotdb/db/storageengine/dataregion/wal/allocation/FirstCreateStrategyTest.java:271 - Default toString(): WALFakeNode inherits toString() from Object, and so is not suitable for printing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.../apache/iotdb/db/storageengine/dataregion/wal/allocation/AbstractNodeAllocationStrategy.java
Show resolved
Hide resolved
Signed-off-by: OneSizeFitsQuorum <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/wal/allocation/AbstractNodeAllocationStrategy.java:91
- The createWALNode overload with four parameters is not synchronized, while the other createWALNode method (line 60) is synchronized. This inconsistency could lead to thread safety issues if both methods are called concurrently and access shared state. For consistency and to prevent potential race conditions, consider making this method synchronized as well, or document why synchronization is not needed for this overload.
protected IWALNode createWALNode(
String identifier, String folder, long startFileVersion, long startSearchIndex) {
try {
return new WALNode(identifier, folder, startFileVersion, startSearchIndex);
} catch (IOException e) {
logger.error("Fail to create wal node", e);
return WALFakeNode.getFailureInstance(e);
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ava/org/apache/iotdb/db/storageengine/dataregion/wal/allocation/FirstCreateStrategyTest.java
Outdated
Show resolved
Hide resolved
…ngine/dataregion/wal/allocation/FirstCreateStrategyTest.java Co-authored-by: Copilot <[email protected]>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.



This PR addresses an NPE issue in WALNode creation that occurs when IoTDB starts with a full disk. When the disk is full at startup, the FolderManager initialization in the constructor fails, leaving it null. If disk space is later freed, subsequent WAL node creation attempts fail with NPE because the folderManager is still null.