diff --git a/server/src/main/java/org/opensearch/index/store/RemoteDirectory.java b/server/src/main/java/org/opensearch/index/store/RemoteDirectory.java index 6534d00662fb4..9b7f42410c469 100644 --- a/server/src/main/java/org/opensearch/index/store/RemoteDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/RemoteDirectory.java @@ -184,6 +184,22 @@ public void deleteFile(String name) throws IOException { blobContainer.deleteBlobsIgnoringIfNotExists(Collections.singletonList(name)); } + /** + * Removes multiple existing files in the directory in a batch operation. + * + *

This method will not throw an exception when a file doesn't exist and simply ignores missing files. + * This is consistent with the behavior of {@link #deleteFile(String)}. + * + * @param names the collection of filenames to delete. + * @throws IOException if the files exist but could not be deleted. + */ + public void deleteFiles(List names) throws IOException { + if (names == null || names.isEmpty()) { + return; + } + blobContainer.deleteBlobsIgnoringIfNotExists(names); + } + /** * Creates and returns a new instance of {@link RemoteIndexOutput} which will be used to copy files to the remote * store. diff --git a/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java b/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java index af8382e2a3154..421020f078c70 100644 --- a/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java @@ -1013,28 +1013,33 @@ public void deleteStaleSegments(int lastNMetadataFilesToKeep) throws IOException .stream() .map(metadata -> metadata.uploadedFilename) .collect(Collectors.toSet()); - AtomicBoolean deletionSuccessful = new AtomicBoolean(true); - staleSegmentRemoteFilenames.stream() + + // Collect all files to delete for this metadata file + List filesToDelete = staleSegmentRemoteFilenames.stream() .filter(file -> activeSegmentRemoteFilenames.contains(file) == false) .filter(file -> deletedSegmentFiles.contains(file) == false) - .forEach(file -> { - try { - remoteDataDirectory.deleteFile(file); - deletedSegmentFiles.add(file); - if (!activeSegmentFilesMetadataMap.containsKey(getLocalSegmentFilename(file))) { - segmentsUploadedToRemoteStore.remove(getLocalSegmentFilename(file)); - } - } catch (NoSuchFileException e) { - logger.info("Segment file {} corresponding to metadata file {} does not exist in remote", file, metadataFile); - } catch (IOException e) { - deletionSuccessful.set(false); - logger.warn( - "Exception while deleting segment file {} corresponding to metadata file {}. Deletion will be re-tried", - file, - metadataFile - ); + .collect(Collectors.toList()); + + AtomicBoolean deletionSuccessful = new AtomicBoolean(true); + try { + // Batch delete all stale segment files + remoteDataDirectory.deleteFiles(filesToDelete); + deletedSegmentFiles.addAll(filesToDelete); + + // Update cache after successful batch deletion + for (String file : filesToDelete) { + if (!activeSegmentFilesMetadataMap.containsKey(getLocalSegmentFilename(file))) { + segmentsUploadedToRemoteStore.remove(getLocalSegmentFilename(file)); } - }); + } + } catch (IOException e) { + deletionSuccessful.set(false); + logger.warn( + "Exception while deleting segment files corresponding to metadata file {}. Deletion will be re-tried", + metadataFile, + e + ); + } if (deletionSuccessful.get()) { logger.debug("Deleting stale metadata file {} from remote segment store", metadataFile); remoteMetadataDirectory.deleteFile(metadataFile); diff --git a/server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java b/server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java index ea6e6e538caa5..abdf3dc6ecc15 100644 --- a/server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java +++ b/server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java @@ -198,6 +198,61 @@ public void testDeleteFileException() throws IOException { assertThrows(IOException.class, () -> remoteDirectory.deleteFile("segment_1")); } + /** + * + * Tests that deleteFiles successfully deletes multiple files from the remote store. + */ + public void testDeleteFiles() throws IOException { + List filesToDelete = List.of("segment_1", "segment_2", "segment_3"); + + remoteDirectory.deleteFiles(filesToDelete); + + verify(blobContainer).deleteBlobsIgnoringIfNotExists(filesToDelete); + } + + /** + * + * Tests that deleteFiles handles empty collection gracefully without attempting any deletions. + */ + public void testDeleteFilesEmptyCollection() throws IOException { + remoteDirectory.deleteFiles(Collections.emptyList()); + + verify(blobContainer, times(0)).deleteBlobsIgnoringIfNotExists(any()); + } + + /** + * + * Tests that deleteFiles handles null collection gracefully without attempting any deletions. + */ + public void testDeleteFilesNullCollection() throws IOException { + remoteDirectory.deleteFiles(null); + verify(blobContainer, times(0)).deleteBlobsIgnoringIfNotExists(any()); + } + + /** + * + * Tests that deleteFiles completes successfully even when some files don't exist. + * The underlying deleteBlobsIgnoringIfNotExists should handle non-existent files gracefully. + */ + public void testDeleteFilesWithNonExistentFiles() throws IOException { + List filesToDelete = List.of("segment_1", "non_existent", "segment_2"); + + remoteDirectory.deleteFiles(filesToDelete); + + verify(blobContainer).deleteBlobsIgnoringIfNotExists(filesToDelete); + } + + /** + * + * Tests that deleteFiles propagates IOException when the underlying blob container operation fails. + */ + public void testDeleteFilesException() throws IOException { + List filesToDelete = List.of("segment_1", "segment_2"); + doThrow(new IOException("Error writing to blob store")).when(blobContainer).deleteBlobsIgnoringIfNotExists(filesToDelete); + + assertThrows(IOException.class, () -> remoteDirectory.deleteFiles(filesToDelete)); + } + public void testCreateOutput() { IndexOutput indexOutput = remoteDirectory.createOutput("segment_1", IOContext.DEFAULT); assertTrue(indexOutput instanceof RemoteIndexOutput); diff --git a/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java b/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java index 0e3e81941bdcf..ebc643e4e8384 100644 --- a/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java +++ b/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java @@ -65,6 +65,7 @@ import static org.opensearch.test.RemoteStoreTestUtils.getDummyMetadata; import static org.hamcrest.CoreMatchers.is; import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.Mockito.any; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; @@ -956,11 +957,11 @@ public void testDeleteStaleCommitsActualDelete() throws Exception { ); final Map> metadataFilenameContentMapping = populateMetadata(); - final List filesToBeDeleted = metadataFilenameContentMapping.get(metadataFilename3) + final Set expectedFilesToDelete = metadataFilenameContentMapping.get(metadataFilename3) .values() .stream() .map(metadata -> metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]) - .collect(Collectors.toList()); + .collect(Collectors.toSet()); remoteSegmentStoreDirectory.init(); @@ -968,10 +969,13 @@ public void testDeleteStaleCommitsActualDelete() throws Exception { // We are passing lastNMetadataFilesToKeep=2 here so that oldest 1 metadata file will be deleted remoteSegmentStoreDirectory.deleteStaleSegmentsAsync(2); - for (final String file : filesToBeDeleted) { - verify(remoteDataDirectory).deleteFile(file); - } - assertBusy(() -> assertThat(remoteSegmentStoreDirectory.canDeleteStaleCommits.get(), is(true))); + // Verify batch deletion was called with the list of files (order-independent) + assertBusy(() -> { + verify(remoteDataDirectory).deleteFiles( + org.mockito.ArgumentMatchers.argThat(files -> files != null && new HashSet<>(files).equals(expectedFilesToDelete)) + ); + assertThat(remoteSegmentStoreDirectory.canDeleteStaleCommits.get(), is(true)); + }); verify(remoteMetadataDirectory).deleteFile(metadataFilename3); appender.assertAllExpectationsMatched(); } @@ -984,15 +988,24 @@ public void testDeleteStaleCommitsActualDeleteWithLocks() throws Exception { // Locking one of the metadata files to ensure that it is not getting deleted. when(mdLockManager.fetchLockedMetadataFiles(any())).thenReturn(Set.of(metadataFilename2)); + // Collect files that should be deleted in batch + Set expectedFilesToDelete = metadataFilenameContentMapping.get(metadataFilename3) + .values() + .stream() + .map(metadata -> metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]) + .collect(Collectors.toSet()); + // popluateMetadata() adds stub to return 3 metadata files // We are passing lastNMetadataFilesToKeep=2 here so that oldest 1 metadata file will be deleted remoteSegmentStoreDirectory.deleteStaleSegmentsAsync(1); - for (String metadata : metadataFilenameContentMapping.get(metadataFilename3).values()) { - String uploadedFilename = metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]; - verify(remoteDataDirectory).deleteFile(uploadedFilename); - } - assertBusy(() -> assertThat(remoteSegmentStoreDirectory.canDeleteStaleCommits.get(), is(true))); + // Verify batch deletion was called (order-independent) + assertBusy(() -> { + verify(remoteDataDirectory).deleteFiles( + org.mockito.ArgumentMatchers.argThat(files -> files != null && new HashSet<>(files).equals(expectedFilesToDelete)) + ); + assertThat(remoteSegmentStoreDirectory.canDeleteStaleCommits.get(), is(true)); + }); verify(remoteMetadataDirectory).deleteFile(metadataFilename3); verify(remoteMetadataDirectory, times(0)).deleteFile(metadataFilename2); } @@ -1049,6 +1062,7 @@ public void testDeleteStaleCommitsDeleteDedup() throws Exception { // We are passing lastNMetadataFilesToKeep=2 here so that oldest 2 metadata files will be deleted remoteSegmentStoreDirectory.deleteStaleSegmentsAsync(2); + // Collect unique stale segment files (deduplication happens in the implementation) Set staleSegmentFiles = new HashSet<>(); for (String metadata : metadataFilenameContentMapping.get(metadataFilename3).values()) { staleSegmentFiles.add(metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]); @@ -1056,15 +1070,23 @@ public void testDeleteStaleCommitsDeleteDedup() throws Exception { for (String metadata : metadataFilenameContentMapping.get(metadataFilename4).values()) { staleSegmentFiles.add(metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]); } - staleSegmentFiles.forEach(file -> { - try { - // Even with the same files in 2 stale metadata files, delete should be called only once. - verify(remoteDataDirectory, times(1)).deleteFile(file); - } catch (IOException e) { - throw new RuntimeException(e); - } + + // Collect expected files to be deleted + Set expectedFilesToDelete_3 = metadataFilenameContentMapping.get(metadataFilename3) + .values() + .stream() + .map(metadata -> metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]) + .collect(Collectors.toSet()); + + assertBusy(() -> { + assertThat(remoteSegmentStoreDirectory.canDeleteStaleCommits.get(), is(true)); + // Verify deleteFiles was called for metadataFilename3 with its files + verify(remoteDataDirectory).deleteFiles( + argThat(files -> files != null && new HashSet<>(files).equals(expectedFilesToDelete_3)) + ); + verify(remoteDataDirectory).deleteFiles(new ArrayList<>()); }); - assertBusy(() -> assertThat(remoteSegmentStoreDirectory.canDeleteStaleCommits.get(), is(true))); + verify(remoteMetadataDirectory).deleteFile(metadataFilename3); verify(remoteMetadataDirectory).deleteFile(metadataFilename4); } @@ -1073,22 +1095,31 @@ public void testDeleteStaleCommitsActualDeleteIOException() throws Exception { Map> metadataFilenameContentMapping = populateMetadata(); remoteSegmentStoreDirectory.init(); - String segmentFileWithException = metadataFilenameContentMapping.get(metadataFilename3) + // Collect files that will be attempted to delete in batch + Set expectedFilesToDelete = metadataFilenameContentMapping.get(metadataFilename3) .values() .stream() - .findAny() - .get() - .split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]; - doThrow(new IOException("Error")).when(remoteDataDirectory).deleteFile(segmentFileWithException); + .map(metadata -> metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]) + .collect(Collectors.toSet()); + + // Make batch deletion throw an exception (order-independent matcher) + doThrow(new IOException("Error")).when(remoteDataDirectory) + .deleteFiles( + org.mockito.ArgumentMatchers.argThat(files -> files != null && new HashSet<>(files).equals(expectedFilesToDelete)) + ); + // popluateMetadata() adds stub to return 3 metadata files // We are passing lastNMetadataFilesToKeep=2 here so that oldest 1 metadata file will be deleted remoteSegmentStoreDirectory.deleteStaleSegmentsAsync(2); - for (String metadata : metadataFilenameContentMapping.get(metadataFilename3).values()) { - String uploadedFilename = metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]; - verify(remoteDataDirectory).deleteFile(uploadedFilename); - } - assertBusy(() -> assertThat(remoteSegmentStoreDirectory.canDeleteStaleCommits.get(), is(true))); + // Verify batch deletion was attempted + assertBusy(() -> { + verify(remoteDataDirectory).deleteFiles( + org.mockito.ArgumentMatchers.argThat(files -> files != null && new HashSet<>(files).equals(expectedFilesToDelete)) + ); + assertThat(remoteSegmentStoreDirectory.canDeleteStaleCommits.get(), is(true)); + }); + // Metadata file should not be deleted when batch deletion fails verify(remoteMetadataDirectory, times(0)).deleteFile(metadataFilename3); } @@ -1096,23 +1127,107 @@ public void testDeleteStaleCommitsActualDeleteNoSuchFileException() throws Excep Map> metadataFilenameContentMapping = populateMetadata(); remoteSegmentStoreDirectory.init(); - String segmentFileWithException = metadataFilenameContentMapping.get(metadataFilename) + // Collect files that will be deleted in batch + Set expectedFilesToDelete = metadataFilenameContentMapping.get(metadataFilename3) .values() .stream() - .findAny() - .get() - .split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]; - doThrow(new NoSuchFileException(segmentFileWithException)).when(remoteDataDirectory).deleteFile(segmentFileWithException); + .map(metadata -> metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]) + .collect(Collectors.toSet()); + + // The batch deleteFiles operation should handle NoSuchFileException gracefully + // (deleteBlobsIgnoringIfNotExists is used internally) + // So we don't throw an exception here - the implementation handles missing files + // popluateMetadata() adds stub to return 3 metadata files // We are passing lastNMetadataFilesToKeep=2 here so that oldest 1 metadata file will be deleted remoteSegmentStoreDirectory.deleteStaleSegmentsAsync(2); - for (String metadata : metadataFilenameContentMapping.get(metadataFilename3).values()) { - String uploadedFilename = metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]; - verify(remoteDataDirectory).deleteFile(uploadedFilename); + // Verify batch deletion was called (order-independent) + assertBusy(() -> { + verify(remoteDataDirectory).deleteFiles(argThat(files -> files != null && new HashSet<>(files).equals(expectedFilesToDelete))); + assertThat(remoteSegmentStoreDirectory.canDeleteStaleCommits.get(), is(true)); + }); + // Metadata file should still be deleted even if some segment files don't exist + verify(remoteMetadataDirectory).deleteFile(metadataFilename3); + } + + /** + * Test that deleteStaleSegments correctly batches file deletions + * Validates that deleteFiles is called with a collection instead of individual deleteFile calls + */ + public void testDeleteStaleSegmentsBatchesDeletions() throws Exception { + Map> metadataFilenameContentMapping = populateMetadata(); + remoteSegmentStoreDirectory.init(); + + // Collect expected files to be deleted + Set expectedFilesToDelete = metadataFilenameContentMapping.get(metadataFilename3) + .values() + .stream() + .map(metadata -> metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]) + .collect(Collectors.toSet()); + + // Execute deletion + remoteSegmentStoreDirectory.deleteStaleSegmentsAsync(2); + + // Verify that deleteFiles was called with the batch of files (order-independent) + assertBusy(() -> { + verify(remoteDataDirectory).deleteFiles(argThat(files -> files != null && new HashSet<>(files).equals(expectedFilesToDelete))); + assertThat(remoteSegmentStoreDirectory.canDeleteStaleCommits.get(), is(true)); + }); + + // Verify metadata file was deleted + verify(remoteMetadataDirectory).deleteFile(metadataFilename3); + + // Get the cache after deletion + Map cacheAfter = remoteSegmentStoreDirectory + .getSegmentsUploadedToRemoteStore(); + // Verify that unique files from deleted metadata are removed from cache + for (String file : expectedFilesToDelete) { + assertFalse("File " + file + " should not be in cache after deletion", cacheAfter.containsKey(file)); } + } + + /** + * Test error handling maintains existing behavior when batch deletion fails + * Validates that when deleteFiles throws an exception, the metadata file is not deleted + * and the cache remains consistent + */ + public void testDeleteStaleSegmentsBatchDeletionErrorHandling() throws Exception { + Map> metadataFilenameContentMapping = populateMetadata(); + remoteSegmentStoreDirectory.init(); + + // Collect files that will be attempted to delete + Set filesToDelete = metadataFilenameContentMapping.get(metadataFilename3) + .values() + .stream() + .map(metadata -> metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]) + .collect(Collectors.toSet()); + + // Make deleteFiles throw an exception for any collection that matches our expected files + doThrow(new IOException("Batch deletion failed")).when(remoteDataDirectory) + .deleteFiles(org.mockito.ArgumentMatchers.argThat(files -> files != null && new HashSet<>(files).equals(filesToDelete))); + + // Execute deletion + remoteSegmentStoreDirectory.deleteStaleSegmentsAsync(2); + + // Wait for async operation to complete assertBusy(() -> assertThat(remoteSegmentStoreDirectory.canDeleteStaleCommits.get(), is(true))); - verify(remoteMetadataDirectory).deleteFile(metadataFilename3); + + // Verify that deleteFiles was called + verify(remoteDataDirectory).deleteFiles( + org.mockito.ArgumentMatchers.argThat(files -> files != null && new HashSet<>(files).equals(filesToDelete)) + ); + + // Verify that metadata file was NOT deleted due to the error + verify(remoteMetadataDirectory, times(0)).deleteFile(metadataFilename3); + + // Verify cache still contains the files (they weren't successfully deleted) + Map cache = remoteSegmentStoreDirectory + .getSegmentsUploadedToRemoteStore(); + + for (String localFile : metadataFilenameContentMapping.get(metadataFilename3).keySet()) { + assertTrue("File " + localFile + " should still be in cache after failed deletion", cache.containsKey(localFile)); + } } public void testSegmentMetadataCurrentVersion() {