server/src/test/java/org/opensearch/index/engine/CompositeIndexWriterForAppendTests.java (1)

192-228: Consider improving test clarity and robustness.

The test verifies that child directories are removed after refresh, but has some areas that could be strengthened:

1. Line 219: The filter logic with path.endsWith("extra0") == false and the - 1 adjustment uses unexplained magic values. This makes the test fragile and harder to maintain.

2. Line 219: Style - attributes.isDirectory() == true should be simplified to attributes.isDirectory().

3. The test doesn't verify that child directories were actually created before refresh—it only checks they're gone after. The test could pass vacuously if directories were never created.

🔎 Suggested improvements

 public void testChildDirectoryDeletedPostRefresh() throws IOException, InterruptedException {
     final EngineConfig engineConfig = config();
     Path dataPath = engineConfig.getStore().shardPath().resolveIndex();
     CompositeIndexWriter compositeIndexWriter = null;
     
     try {
         compositeIndexWriter = new CompositeIndexWriter(
             engineConfig,
             createWriter(),
             newSoftDeletesPolicy(),
             softDeletesField,
             indexWriterFactory
         );
         
         Engine.Index operation = indexForDoc(createParsedDoc("id", null, DEFAULT_CRITERIA));
         try (Releasable ignore1 = compositeIndexWriter.acquireLock(operation.uid().bytes())) {
             compositeIndexWriter.addDocuments(operation.docs(), operation.uid());
         }
         
         operation = indexForDoc(createParsedDoc("id2", null, "testingNewCriteria"));
         try (Releasable ignore1 = compositeIndexWriter.acquireLock(operation.uid().bytes())) {
             compositeIndexWriter.addDocuments(operation.docs(), operation.uid());
         }
         
+        // Verify child directories were created
+        long childDirCountBefore = Files.list(dataPath)
+            .filter(path -> Files.isDirectory(path) && path.getFileName().toString().startsWith(CHILD_DIRECTORY_PREFIX))
+            .count();
+        assertTrue("Expected child directories to be created before refresh", childDirCountBefore > 0);
+        
         compositeIndexWriter.beforeRefresh();
         compositeIndexWriter.afterRefresh(true);
         
-        long directoryCount = Files.find(dataPath, 1, (path, attributes) -> attributes.isDirectory() == true && path.endsWith("extra0") == false).count() - 1;
-        // Ensure no child directory is pending here.
-        assertEquals(0, directoryCount);
+        // Verify all child directories are removed after refresh
+        long childDirCountAfter = Files.list(dataPath)
+            .filter(path -> Files.isDirectory(path) && path.getFileName().toString().startsWith(CHILD_DIRECTORY_PREFIX))
+            .count();
+        assertEquals("Expected no child directories after refresh", 0, childDirCountAfter);
     }  finally {
         if (compositeIndexWriter != null) {
             IOUtils.closeWhileHandlingException(compositeIndexWriter);
         }
     }
 }

Configuration used: defaults

Review profile: CHILL

Plan: Pro

server/src/main/java/org/opensearch/index/engine/CompositeIndexWriter.java (2)

546-550: Error handling needs improvement for directory cleanup.

The current implementation has the issues identified in previous reviews:

1. If getLocalFSDirectory throws during the stream mapping (line 546-547), the directories in directoryToCombine remain open and won't be closed
2. If IOUtils.rm fails (line 550), it triggers a full rollback via the catch block at line 529, causing shard failure even though the merge succeeded and directories are already closed

This can lead to:

• Orphaned closed directories accumulating on disk
• Unnecessary shard failures from transient filesystem issues (permissions, disk space)

Consider wrapping the path extraction in a try-finally block to ensure directories are always closed, and handle removal failures more gracefully (either using IOUtils.deleteFilesIgnoringExceptions or logging failures without triggering rollback).

Based on previous review feedback that flagged these same concerns.

---

556-566: Add defensive checks to prevent ClassCastException.

The method lacks validation and can fail with ClassCastException in several scenarios:

1. No null check on localDirectory
2. No validation before casting to FilterDirectory (line 562)
3. No validation that the delegate is actually FSDirectory (line 562)
4. Only unwraps one level of FilterDirectory (multiple layers not handled)
5. Comment mentions "as per above validation" but no such validation exists

Consider adding:

• Null check at the start
• Validation that localDirectory is either FSDirectory or FilterDirectory, with a clear exception message if not
• Recursive unwrapping to handle multiple FilterDirectory layers
• Validation that the final unwrapped delegate is FSDirectory

Based on previous review feedback that identified these missing defensive checks.

server/src/test/java/org/opensearch/index/engine/CompositeIndexWriterForAppendTests.java (1)

192-232: Consider strengthening the test verification.

The test validates the core functionality, but could be more robust:

1. Line 222: The filter excluding "extra0" relies on hardcoded internal directory naming, which is brittle
2. Line 223: Subtracting 1 from the count assumes a specific directory structure that may change
3. Missing verification: The test doesn't confirm child directories existed before refresh (only checks they're gone after)
4. No document validation: Doesn't verify the documents were successfully indexed/merged

Consider:

• Counting child directories before and after refresh to confirm cleanup actually occurred
• Using a more robust filter (e.g., checking for the CHILD_DIRECTORY_PREFIX constant from line 56 of CompositeIndexWriter)
• Verifying the documents are accessible after refresh via a DirectoryReader

🔎 Example improvement

+// Count child directories before refresh
+long preRefreshCount = Files.find(
+    dataPath,
+    1, 
+    (path, attrs) -> attrs.isDirectory() && path.getFileName().toString().startsWith(BucketedCompositeDirectory.CHILD_DIRECTORY_PREFIX)
+).count();
+assertTrue("Expected child directories before refresh", preRefreshCount > 0);
+
 compositeIndexWriter.beforeRefresh();
 compositeIndexWriter.afterRefresh(true);
 
-long directoryCount = Files.find(
+long postRefreshCount = Files.find(
     dataPath,
     1,
-    (path, attributes) -> attributes.isDirectory() == true && path.endsWith("extra0") == false
-).count() - 1;
-// Ensure no child directory is pending here.
-assertEquals(0, directoryCount);
+    (path, attrs) -> attrs.isDirectory() && path.getFileName().toString().startsWith(BucketedCompositeDirectory.CHILD_DIRECTORY_PREFIX)
+).count();
+assertEquals("Child directories should be cleaned up after refresh", 0, postRefreshCount);
+
+// Verify documents are still accessible
+try (DirectoryReader reader = DirectoryReader.open(compositeIndexWriter.getAccumulatingIndexWriter())) {
+    assertEquals("Both documents should be in parent writer", 2, reader.numDocs());
+}

Configuration used: defaults

Review profile: CHILL

Plan: Pro