From 5ebf35cdfd8df54492a4ad03100d85b756b32c25 Mon Sep 17 00:00:00 2001 From: Gaurav Bafna Date: Tue, 2 Dec 2025 11:48:08 +0530 Subject: [PATCH 1/3] Batch deletes in remote segment store Signed-off-by: Gaurav Bafna --- .../index/store/RemoteDirectory.java | 16 ++ .../store/RemoteSegmentStoreDirectory.java | 43 ++-- .../index/store/RemoteDirectoryTests.java | 55 +++++ .../RemoteSegmentStoreDirectoryTests.java | 191 ++++++++++++++---- 4 files changed, 248 insertions(+), 57 deletions(-) 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() { From 74ac91a4d7763081afb1e15fc127aa5b1426589b Mon Sep 17 00:00:00 2001 From: Gaurav Bafna Date: Thu, 11 Dec 2025 15:40:20 +0530 Subject: [PATCH 2/3] address PR comments --- .../opensearch/index/store/RemoteSegmentStoreDirectory.java | 6 ++++-- .../index/store/BaseRemoteSegmentStoreDirectoryTests.java | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) 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 421020f078c70..bb3a95f1908f2 100644 --- a/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java @@ -1035,8 +1035,10 @@ public void deleteStaleSegments(int lastNMetadataFilesToKeep) throws IOException } catch (IOException e) { deletionSuccessful.set(false); logger.warn( - "Exception while deleting segment files corresponding to metadata file {}. Deletion will be re-tried", - metadataFile, + () -> new ParameterizedMessage( + "Exception while deleting segment files corresponding to metadata file {}. Deletion will be re-tried", + metadataFile + ), e ); } diff --git a/server/src/test/java/org/opensearch/index/store/BaseRemoteSegmentStoreDirectoryTests.java b/server/src/test/java/org/opensearch/index/store/BaseRemoteSegmentStoreDirectoryTests.java index 284bd924106ea..1c5dec20fd74a 100644 --- a/server/src/test/java/org/opensearch/index/store/BaseRemoteSegmentStoreDirectoryTests.java +++ b/server/src/test/java/org/opensearch/index/store/BaseRemoteSegmentStoreDirectoryTests.java @@ -113,9 +113,9 @@ protected Map> populateMetadata() throws IOException metadataFilename, getDummyMetadata("_0", 1), metadataFilename2, - getDummyMetadata("_0", 1), + getDummyMetadata("_0", 2), metadataFilename3, - getDummyMetadata("_0", 1) + getDummyMetadata("_0", 3) ); when(remoteMetadataDirectory.getBlobStream(metadataFilename)).thenAnswer( From 8f3bb704c02887bdbca0950d71d59db9f93762df Mon Sep 17 00:00:00 2001 From: Gaurav Bafna Date: Tue, 16 Dec 2025 13:44:05 +0530 Subject: [PATCH 3/3] fix tests --- .../RemoteSegmentStoreDirectoryTests.java | 55 +++++++++++++------ ...toreDirectoryWithPinnedTimestampTests.java | 41 ++++++++++++-- 2 files changed, 75 insertions(+), 21 deletions(-) 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 ebc643e4e8384..373f253b4d205 100644 --- a/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java +++ b/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java @@ -957,6 +957,7 @@ public void testDeleteStaleCommitsActualDelete() throws Exception { ); final Map> metadataFilenameContentMapping = populateMetadata(); + // Collect files to delete from all metadata files final Set expectedFilesToDelete = metadataFilenameContentMapping.get(metadataFilename3) .values() .stream() @@ -1166,6 +1167,10 @@ public void testDeleteStaleSegmentsBatchesDeletions() throws Exception { .map(metadata -> metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]) .collect(Collectors.toSet()); + // Get the cache after deletion + Map cacheBefore = remoteSegmentStoreDirectory + .getSegmentsUploadedToRemoteStore(); + // Execute deletion remoteSegmentStoreDirectory.deleteStaleSegmentsAsync(2); @@ -1177,14 +1182,36 @@ public void testDeleteStaleSegmentsBatchesDeletions() throws Exception { // Verify metadata file was deleted verify(remoteMetadataDirectory).deleteFile(metadataFilename3); + } + + public void testDeleteStaleSegmentsBatchesDeletions_cache() throws Exception { + Map> metadataFilenameContentMapping = populateMetadata(); + remoteSegmentStoreDirectory.init(); + + // Collect expected files to be deleted + Set expectedFilesToDelete = metadataFilenameContentMapping.get(metadataFilename) + .values() + .stream() + .map(metadata -> metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]) + .collect(Collectors.toSet()); + + // Execute deletion + remoteSegmentStoreDirectory.deleteStaleSegmentsAsync(0); + + // 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)); - } + + assertEquals(0, cacheAfter.size()); } /** @@ -1197,36 +1224,32 @@ public void testDeleteStaleSegmentsBatchDeletionErrorHandling() throws Exception remoteSegmentStoreDirectory.init(); // Collect files that will be attempted to delete - Set filesToDelete = metadataFilenameContentMapping.get(metadataFilename3) + Set filesToDelete = metadataFilenameContentMapping.get(metadataFilename) .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))); + doThrow(new IOException("Batch deletion failed")).when(remoteDataDirectory).deleteFiles(org.mockito.ArgumentMatchers.any()); // Execute deletion - remoteSegmentStoreDirectory.deleteStaleSegmentsAsync(2); + remoteSegmentStoreDirectory.deleteStaleSegmentsAsync(0); // Wait for async operation to complete assertBusy(() -> assertThat(remoteSegmentStoreDirectory.canDeleteStaleCommits.get(), is(true))); - // 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(remoteMetadataDirectory, times(0)).deleteFile(metadataFilename); // 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)); + for (String localFile : metadataFilenameContentMapping.get(metadataFilename).keySet()) { + if (localFile.contains("segments")) { + assertTrue("File " + localFile + " should still be in cache after failed deletion", cache.containsKey(localFile)); + } } } diff --git a/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryWithPinnedTimestampTests.java b/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryWithPinnedTimestampTests.java index 2531462d21d40..48a71c55bb8ce 100644 --- a/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryWithPinnedTimestampTests.java +++ b/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryWithPinnedTimestampTests.java @@ -24,8 +24,10 @@ import java.io.IOException; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -34,6 +36,7 @@ import static org.opensearch.indices.RemoteStoreSettings.CLUSTER_REMOTE_STORE_PINNED_TIMESTAMP_ENABLED; import static org.hamcrest.CoreMatchers.is; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.Mockito.anyInt; import static org.mockito.Mockito.eq; import static org.mockito.Mockito.mock; @@ -193,11 +196,11 @@ public void testDeleteStaleCommitsPinnedTimestampMdFile() throws Exception { when(blobContainer.listBlobs()).thenReturn(Map.of(blobName, new PlainBlobMetadata(blobName, 100))); final Map> metadataFilenameContentMapping = populateMetadata(); - final List filesToBeDeleted = metadataFilenameContentMapping.get(metadataFilename3) + final Set filesToBeDeleted = metadataFilenameContentMapping.get(metadataFilename3) .values() .stream() .map(metadata -> metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]) - .collect(Collectors.toList()); + .collect(Collectors.toSet()); updatePinnedTimstampTask.run(); @@ -207,11 +210,39 @@ public void testDeleteStaleCommitsPinnedTimestampMdFile() throws Exception { // We are passing lastNMetadataFilesToKeep=2 here so that oldest 1 metadata file will be deleted remoteSegmentStoreDirectory.deleteStaleSegmentsAsync(1); - for (final String file : filesToBeDeleted) { - verify(remoteDataDirectory).deleteFile(file); - } + // 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(filesToBeDeleted)) + ); + }); + assertBusy(() -> assertThat(remoteSegmentStoreDirectory.canDeleteStaleCommits.get(), is(true))); verify(remoteMetadataDirectory).deleteFile(metadataFilename3); verify(remoteMetadataDirectory, times(0)).deleteFile(metadataFilename2); } + + public void testDeleteStaleSegmentsBatchesDeletions_cache() throws Exception { + Map> metadataFilenameContentMapping = populateMetadata(); + remoteSegmentStoreDirectory.init(); + + // Collect expected files to be deleted + Set expectedFilesToDelete = metadataFilenameContentMapping.get(metadataFilename2) + .values() + .stream() + .map(metadata -> metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]) + .collect(Collectors.toSet()); + + // Execute deletion + remoteSegmentStoreDirectory.deleteStaleSegmentsAsync(0); + + // 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(metadataFilename2); + } }