Copilot commented on code in PR #16869:
URL: https://github.com/apache/iotdb/pull/16869#discussion_r2615996321
##########
iotdb-core/datanode/src/test/java/org/apache/iotdb/db/storageengine/dataregion/wal/allocation/FirstCreateStrategyTest.java:
##########
@@ -141,6 +142,79 @@ public void testRegisterWALNode() throws
IllegalPathException {
}
}
+ @Test
+ public void testReInitializeAfterDiskSpaceCleaned() throws
IllegalPathException, IOException {
+ // Create temporary directories for testing
+ File tempDir = new File(System.getProperty("java.io.tmpdir"),
"iotdb_wal_reinit_test");
+ tempDir.mkdirs();
+
+ String[] testWalDirs =
+ new String[] {
+ new File(tempDir, "wal_reinit_test1").getAbsolutePath(),
+ new File(tempDir, "wal_reinit_test2").getAbsolutePath(),
+ new File(tempDir, "wal_reinit_test3").getAbsolutePath()
+ };
+
+ String[] originalWalDirs = commonConfig.getWalDirs();
+ commonConfig.setWalDirs(testWalDirs);
+
+ try {
+ // Create strategy with valid directories first
+ FirstCreateStrategy strategy = new FirstCreateStrategy();
+
+ // Simulate folderManager becoming null (e.g., due to disk space issues)
+ // We'll use reflection to set folderManager to null to test
re-initialization
+ try {
+ java.lang.reflect.Field folderManagerField =
+
AbstractNodeAllocationStrategy.class.getDeclaredField("folderManager");
+ folderManagerField.setAccessible(true);
+ folderManagerField.set(strategy, null);
+ } catch (NoSuchFieldException | IllegalAccessException e) {
+ throw new RuntimeException("Failed to set folderManager to null for
testing", e);
+ }
+
+ // Now apply for WAL node, should successfully re-initialize
folderManager
+ IWALNode walNode = strategy.applyForWALNode("test_reinit_identifier");
+ assertNotNull("WAL node should be created after re-initialization",
walNode);
Review Comment:
The test verifies that a WAL node is created after re-initialization, but it
doesn't verify that the re-initialization actually occurred. It only checks
that walNode is not null, which could also be true if a WALFakeNode failure
instance was returned. Consider adding an assertion to verify that the returned
node is an instance of WALNode (not WALFakeNode) to ensure the
re-initialization was truly successful.
##########
iotdb-core/datanode/src/test/java/org/apache/iotdb/db/storageengine/dataregion/wal/allocation/FirstCreateStrategyTest.java:
##########
@@ -141,6 +142,79 @@ public void testRegisterWALNode() throws
IllegalPathException {
}
}
+ @Test
+ public void testReInitializeAfterDiskSpaceCleaned() throws
IllegalPathException, IOException {
+ // Create temporary directories for testing
+ File tempDir = new File(System.getProperty("java.io.tmpdir"),
"iotdb_wal_reinit_test");
+ tempDir.mkdirs();
Review Comment:
The test creates a temporary directory but uses a hardcoded name
"iotdb_wal_reinit_test" which could cause conflicts if multiple test instances
run concurrently or if a previous test run failed to clean up. Consider using a
unique temporary directory name (e.g., with timestamp or UUID) or Java's
Files.createTempDirectory() to avoid potential conflicts.
##########
iotdb-core/datanode/src/test/java/org/apache/iotdb/db/storageengine/dataregion/wal/allocation/FirstCreateStrategyTest.java:
##########
@@ -141,6 +142,79 @@ public void testRegisterWALNode() throws
IllegalPathException {
}
}
+ @Test
+ public void testReInitializeAfterDiskSpaceCleaned() throws
IllegalPathException, IOException {
+ // Create temporary directories for testing
+ File tempDir = new File(System.getProperty("java.io.tmpdir"),
"iotdb_wal_reinit_test");
+ tempDir.mkdirs();
+
+ String[] testWalDirs =
+ new String[] {
+ new File(tempDir, "wal_reinit_test1").getAbsolutePath(),
+ new File(tempDir, "wal_reinit_test2").getAbsolutePath(),
+ new File(tempDir, "wal_reinit_test3").getAbsolutePath()
+ };
+
+ String[] originalWalDirs = commonConfig.getWalDirs();
+ commonConfig.setWalDirs(testWalDirs);
+
+ try {
+ // Create strategy with valid directories first
+ FirstCreateStrategy strategy = new FirstCreateStrategy();
+
+ // Simulate folderManager becoming null (e.g., due to disk space issues)
+ // We'll use reflection to set folderManager to null to test
re-initialization
+ try {
+ java.lang.reflect.Field folderManagerField =
+
AbstractNodeAllocationStrategy.class.getDeclaredField("folderManager");
+ folderManagerField.setAccessible(true);
+ folderManagerField.set(strategy, null);
+ } catch (NoSuchFieldException | IllegalAccessException e) {
+ throw new RuntimeException("Failed to set folderManager to null for
testing", e);
+ }
+
+ // Now apply for WAL node, should successfully re-initialize
folderManager
+ IWALNode walNode = strategy.applyForWALNode("test_reinit_identifier");
+ assertNotNull("WAL node should be created after re-initialization",
walNode);
+
+ // Verify that WAL node was created successfully by logging data
+ walNode.log(1, getInsertRowNode());
+
+ // Verify that WAL files were created in at least one directory
+ boolean walFileCreated = false;
+ for (String walDir : testWalDirs) {
+ File walDirFile = new File(walDir);
+ if (walDirFile.exists()) {
+ File[] nodeDirs = walDirFile.listFiles(File::isDirectory);
+ if (nodeDirs != null && nodeDirs.length > 0) {
+ for (File nodeDir : nodeDirs) {
+ if (nodeDir.exists() &&
WALFileUtils.listAllWALFiles(nodeDir).length > 0) {
+ walFileCreated = true;
+ break;
+ }
+ }
+ }
+ }
+ if (walFileCreated) {
+ break;
+ }
+ }
+ assertTrue("WAL files should be created after re-initialization",
walFileCreated);
+
+ // Clean up
+ walNode.close();
+ } finally {
+ // Clean up the test directories
+ for (String walDir : testWalDirs) {
+ EnvironmentUtils.cleanDir(walDir);
+ }
+ // Clean up temp directory
+ EnvironmentUtils.cleanDir(tempDir.getAbsolutePath());
+ // Restore original WAL directories
+ commonConfig.setWalDirs(originalWalDirs);
+ }
+ }
Review Comment:
The test validates successful re-initialization but doesn't test the failure
scenario where re-initialization also fails (e.g., disk still full). This is a
critical edge case because if the folderManager initialization fails both in
the constructor and in createWALNode, the system behavior should still be
predictable. Consider adding a test case that verifies the system returns
WALFakeNode.getFailureInstance when re-initialization fails with
DiskSpaceInsufficientException.
--
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]
