Copilot commented on code in PR #16869:
URL: https://github.com/apache/iotdb/pull/16869#discussion_r2600734733
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/wal/allocation/AbstractNodeAllocationStrategy.java:
##########
@@ -57,8 +58,16 @@ protected AbstractNodeAllocationStrategy() {
protected IWALNode createWALNode(String identifier) {
try {
- return folderManager.getNextWithRetry(
- folder -> new WALNode(identifier, folder + File.separator +
identifier));
+ // already in lock, so no need to synchronized
+ if (folderManager.get() == null) {
+ folderManager.set(
+ new FolderManager(
+ Arrays.asList(commonConfig.getWalDirs()),
DirectoryStrategyType.SEQUENCE_STRATEGY));
Review Comment:
Race condition: Multiple threads can simultaneously check
`folderManager.get() == null` and proceed to create multiple FolderManager
instances. The comment on line 61 states "already in lock, so no need to
synchronized", but AtomicReference alone doesn't prevent this race condition.
The check-then-set pattern is not atomic. Consider using synchronized
double-checked locking:
```java
if (folderManager.get() == null) {
synchronized (folderManager) {
if (folderManager.get() == null) {
folderManager.set(new FolderManager(...));
}
}
}
```
Alternatively, use `compareAndSet()` for atomic updates.
```suggestion
// Atomically initialize folderManager if null
while (folderManager.get() == null) {
FolderManager newManager = new FolderManager(
Arrays.asList(commonConfig.getWalDirs()),
DirectoryStrategyType.SEQUENCE_STRATEGY);
if (folderManager.compareAndSet(null, newManager)) {
break;
}
// else, another thread set it, retry check
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/wal/allocation/AbstractNodeAllocationStrategy.java:
##########
@@ -57,8 +58,16 @@ protected AbstractNodeAllocationStrategy() {
protected IWALNode createWALNode(String identifier) {
try {
- return folderManager.getNextWithRetry(
- folder -> new WALNode(identifier, folder + File.separator +
identifier));
+ // already in lock, so no need to synchronized
+ if (folderManager.get() == null) {
+ folderManager.set(
+ new FolderManager(
+ Arrays.asList(commonConfig.getWalDirs()),
DirectoryStrategyType.SEQUENCE_STRATEGY));
+ }
Review Comment:
Missing test coverage for the disk recovery scenario: The new lazy
initialization logic (lines 62-66) that handles disk space recovery is not
covered by tests. The existing tests in FirstCreateStrategyTest don't simulate
a disk-full-at-startup followed by disk-space-recovery scenario. Consider
adding a test that:
1. Simulates disk full at construction time (folderManager becomes null)
2. Verifies WALFakeNode is returned initially
3. Frees disk space
4. Verifies successful WAL node creation on retry (folderManager is
recreated)
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/wal/allocation/AbstractNodeAllocationStrategy.java:
##########
@@ -57,8 +58,16 @@ protected AbstractNodeAllocationStrategy() {
protected IWALNode createWALNode(String identifier) {
try {
- return folderManager.getNextWithRetry(
- folder -> new WALNode(identifier, folder + File.separator +
identifier));
+ // already in lock, so no need to synchronized
+ if (folderManager.get() == null) {
+ folderManager.set(
+ new FolderManager(
+ Arrays.asList(commonConfig.getWalDirs()),
DirectoryStrategyType.SEQUENCE_STRATEGY));
+ }
+ return folderManager
+ .get()
+ .getNextWithRetry(
+ folder -> new WALNode(identifier, folder + File.separator +
identifier));
Review Comment:
Potential NPE: Even after the null check and re-initialization attempt
(lines 62-66), `folderManager.get()` could still be null if the FolderManager
constructor throws `DiskSpaceInsufficientException` again. In this case, line
67 will throw NPE when calling `.get().getNextWithRetry()`. The exception from
line 63-65 is not caught here, so it would propagate up, but if it were caught
elsewhere, this would be an NPE risk. Consider adding a null check after the
initialization attempt or handling the case where re-initialization fails.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
