diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStorePinnedTimestampsGarbageCollectionIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStorePinnedTimestampsGarbageCollectionIT.java index 15c52fdc03b9a..46d43926e7e34 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStorePinnedTimestampsGarbageCollectionIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStorePinnedTimestampsGarbageCollectionIT.java @@ -112,7 +112,7 @@ public void testLiveIndexNoPinnedTimestamps() throws Exception { assertBusy(() -> { List metadataFiles = Files.list(translogMetadataPath).collect(Collectors.toList()); - assertEquals(1, metadataFiles.size()); + assertEquals(2, metadataFiles.size()); verifyTranslogDataFileCount(metadataFiles, translogDataPath); }); @@ -222,7 +222,7 @@ public void testLiveIndexNoPinnedTimestampsWithExtraGenSetting() throws Exceptio assertBusy(() -> { List metadataFiles = Files.list(translogMetadataPath).collect(Collectors.toList()); - assertEquals(4, metadataFiles.size()); + assertEquals(5, metadataFiles.size()); verifyTranslogDataFileCount(metadataFiles, translogDataPath); }); @@ -282,7 +282,7 @@ public void testLiveIndexWithPinnedTimestamps() throws Exception { assertBusy(() -> { List metadataFiles = Files.list(translogMetadataPath).collect(Collectors.toList()); - assertEquals(2, metadataFiles.size()); + assertEquals(3, metadataFiles.size()); verifyTranslogDataFileCount(metadataFiles, translogDataPath); }); @@ -337,7 +337,7 @@ public void testIndexDeletionNoPinnedTimestamps() throws Exception { assertBusy(() -> { List metadataFiles = Files.list(translogMetadataPath).collect(Collectors.toList()); - assertEquals(1, metadataFiles.size()); + assertEquals(2, metadataFiles.size()); verifyTranslogDataFileCount(metadataFiles, translogDataPath); }); @@ -405,7 +405,7 @@ public void testIndexDeletionWithPinnedTimestamps() throws Exception { assertBusy(() -> { List metadataFiles = Files.list(translogMetadataPath).collect(Collectors.toList()); - assertEquals(2, metadataFiles.size()); + assertEquals(3, metadataFiles.size()); verifyTranslogDataFileCount(metadataFiles, translogDataPath); }, 30, TimeUnit.SECONDS); diff --git a/server/src/main/java/org/opensearch/index/translog/RemoteFsTimestampAwareTranslog.java b/server/src/main/java/org/opensearch/index/translog/RemoteFsTimestampAwareTranslog.java index 92b09188eb1ce..7f52110843645 100644 --- a/server/src/main/java/org/opensearch/index/translog/RemoteFsTimestampAwareTranslog.java +++ b/server/src/main/java/org/opensearch/index/translog/RemoteFsTimestampAwareTranslog.java @@ -203,7 +203,7 @@ public void onResponse(List blobMetadata) { Set generationsToBeDeleted = getGenerationsToBeDeleted( metadataFilesNotToBeDeleted, metadataFilesToBeDeleted, - indexDeleted ? Long.MAX_VALUE : getMinGenerationToKeep() + indexDeleted ? Long.MAX_VALUE : getMinGenerationToKeepInRemote() ); logger.debug(() -> "generationsToBeDeleted = " + generationsToBeDeleted); @@ -246,7 +246,7 @@ public void onFailure(Exception e) { translogTransferManager.listTranslogMetadataFilesAsync(listMetadataFilesListener); } - private long getMinGenerationToKeep() { + private long getMinGenerationToKeepInRemote() { return minRemoteGenReferenced - indexSettings().getRemoteTranslogExtraKeep(); } @@ -254,7 +254,7 @@ private long getMinGenerationToKeep() { protected Set getGenerationsToBeDeleted( List metadataFilesNotToBeDeleted, List metadataFilesToBeDeleted, - long minGenerationToKeep + long minGenerationToKeepInRemote ) throws IOException { Set generationsFromMetadataFilesToBeDeleted = new HashSet<>(); for (String mdFile : metadataFilesToBeDeleted) { @@ -271,7 +271,7 @@ protected Set getGenerationsToBeDeleted( // Check if the generation is not referred by metadata file matching pinned timestamps // The check with minGenerationToKeep is redundant but kept as to make sure we don't delete generations // that are not persisted in remote segment store yet. - if (generation < minGenerationToKeep && isGenerationPinned(generation, pinnedGenerations) == false) { + if (generation < minGenerationToKeepInRemote && isGenerationPinned(generation, pinnedGenerations) == false) { generationsToBeDeleted.add(generation); } } @@ -279,14 +279,20 @@ protected Set getGenerationsToBeDeleted( } protected List getMetadataFilesToBeDeleted(List metadataFiles, boolean indexDeleted) { - return getMetadataFilesToBeDeleted(metadataFiles, metadataFilePinnedTimestampMap, getMinGenerationToKeep(), indexDeleted, logger); + return getMetadataFilesToBeDeleted( + metadataFiles, + metadataFilePinnedTimestampMap, + getMinGenerationToKeepInRemote(), + indexDeleted, + logger + ); } // Visible for testing protected static List getMetadataFilesToBeDeleted( List metadataFiles, Map metadataFilePinnedTimestampMap, - long minGenerationToKeep, + long minGenerationToKeepInRemote, boolean indexDeleted, Logger logger ) { @@ -327,7 +333,7 @@ protected static List getMetadataFilesToBeDeleted( // Filter out metadata files based on minGenerationToKeep List metadataFilesContainingMinGenerationToKeep = metadataFilesToBeDeleted.stream().filter(md -> { long maxGeneration = TranslogTransferMetadata.getMaxGenerationFromFileName(md); - return maxGeneration == -1 || maxGeneration > minGenerationToKeep; + return maxGeneration == -1 || maxGeneration >= minGenerationToKeepInRemote; }).collect(Collectors.toList()); metadataFilesToBeDeleted.removeAll(metadataFilesContainingMinGenerationToKeep); @@ -335,7 +341,7 @@ protected static List getMetadataFilesToBeDeleted( "metadataFilesContainingMinGenerationToKeep.size = {}, metadataFilesToBeDeleted based on minGenerationToKeep filtering = {}, minGenerationToKeep = {}", metadataFilesContainingMinGenerationToKeep.size(), metadataFilesToBeDeleted.size(), - minGenerationToKeep + minGenerationToKeepInRemote ); } diff --git a/server/src/test/java/org/opensearch/index/translog/RemoteFsTimestampAwareTranslogTests.java b/server/src/test/java/org/opensearch/index/translog/RemoteFsTimestampAwareTranslogTests.java index 2a97c42346d31..e6871414cf5e0 100644 --- a/server/src/test/java/org/opensearch/index/translog/RemoteFsTimestampAwareTranslogTests.java +++ b/server/src/test/java/org/opensearch/index/translog/RemoteFsTimestampAwareTranslogTests.java @@ -335,29 +335,46 @@ public void testSimpleOperationsUpload() throws Exception { addToTranslogAndListAndUpload(translog, ops, new Translog.Index("2", 2, primaryTerm.get(), new byte[] { 1 })); addToTranslogAndListAndUpload(translog, ops, new Translog.Index("3", 3, primaryTerm.get(), new byte[] { 1 })); - addToTranslogAndListAndUpload(translog, ops, new Translog.Index("4", 4, primaryTerm.get(), new byte[] { 1 })); - addToTranslogAndListAndUpload(translog, ops, new Translog.Index("5", 5, primaryTerm.get(), new byte[] { 1 })); - addToTranslogAndListAndUpload(translog, ops, new Translog.Index("6", 6, primaryTerm.get(), new byte[] { 1 })); assertBusy(() -> { assertEquals( - 16, + 10, blobStoreTransferService.listAll(getTranslogDirectory().add(DATA_DIR).add(String.valueOf(primaryTerm.get()))).size() ); }); - assertBusy(() -> assertTrue(translog.isRemoteGenerationDeletionPermitsAvailable())); - RemoteStoreSettings.setPinnedTimestampsLookbackInterval(TimeValue.ZERO); + assertBusy(() -> assertTrue(translog.isRemoteGenerationDeletionPermitsAvailable())); // Fetch pinned timestamps so that it won't be stale updatePinnedTimstampTask.run(); + translog.setMinSeqNoToKeep(3); + translog.trimUnreferencedReaders(); + addToTranslogAndListAndUpload(translog, ops, new Translog.Index("4", 4, primaryTerm.get(), new byte[] { 1 })); + addToTranslogAndListAndUpload(translog, ops, new Translog.Index("5", 5, primaryTerm.get(), new byte[] { 1 })); + addToTranslogAndListAndUpload(translog, ops, new Translog.Index("6", 6, primaryTerm.get(), new byte[] { 1 })); + + assertBusy(() -> assertTrue(translog.isRemoteGenerationDeletionPermitsAvailable())); + // Fetch pinned timestamps so that it won't be stale + updatePinnedTimstampTask.run(); translog.setMinSeqNoToKeep(6); translog.trimUnreferencedReaders(); + assertBusy(() -> assertTrue(translog.isRemoteGenerationDeletionPermitsAvailable())); + + assertEquals(1, translog.readers.size()); + assertBusy(() -> { + assertEquals(2, translog.allUploaded().size()); + assertEquals(4, blobStoreTransferService.listAll(getTranslogDirectory().add(METADATA_DIR)).size()); + assertEquals( + 16, + blobStoreTransferService.listAll(getTranslogDirectory().add(DATA_DIR).add(String.valueOf(primaryTerm.get()))).size() + ); + }, 30, TimeUnit.SECONDS); + addToTranslogAndListAndUpload(translog, ops, new Translog.Index("7", 7, primaryTerm.get(), new byte[] { 1 })); addToTranslogAndListAndUpload(translog, ops, new Translog.Index("8", 8, primaryTerm.get(), new byte[] { 1 })); - assertBusy(() -> assertTrue(translog.isRemoteGenerationDeletionPermitsAvailable())); + assertBusy(() -> assertTrue(translog.isRemoteGenerationDeletionPermitsAvailable())); // Fetch pinned timestamps so that it won't be stale updatePinnedTimstampTask.run(); translog.trimUnreferencedReaders(); @@ -365,13 +382,13 @@ public void testSimpleOperationsUpload() throws Exception { assertEquals(3, translog.readers.size()); assertBusy(() -> { - assertEquals(2, blobStoreTransferService.listAll(getTranslogDirectory().add(METADATA_DIR)).size()); assertEquals(6, translog.allUploaded().size()); + assertEquals(3, blobStoreTransferService.listAll(getTranslogDirectory().add(METADATA_DIR)).size()); assertEquals( - 6, + 12, blobStoreTransferService.listAll(getTranslogDirectory().add(DATA_DIR).add(String.valueOf(primaryTerm.get()))).size() ); - }, 60, TimeUnit.SECONDS); + }, 30, TimeUnit.SECONDS); } @Override @@ -573,7 +590,7 @@ public void testDrainSync() throws Exception { assertBusy(() -> assertTrue(translog.isRemoteGenerationDeletionPermitsAvailable())); assertEquals(1, translog.readers.size()); assertBusy(() -> assertEquals(2, translog.allUploaded().size())); - assertBusy(() -> assertEquals(1, blobStoreTransferService.listAll(getTranslogDirectory().add(METADATA_DIR)).size())); + assertBusy(() -> assertEquals(2, blobStoreTransferService.listAll(getTranslogDirectory().add(METADATA_DIR)).size())); } @Override @@ -816,7 +833,7 @@ public void testGetMetadataFilesToBeDeletedExclusionBasedOnGenerationOnly() thro // MaxGen 12 "metadata__9223372036438563903__9223372036854775795__" + md2Timestamp + "__31__9223372036854775803__1", // MaxGen 10 - "metadata__9223372036438563903__9223372036854775797__" + md3Timestamp + "__31__9223372036854775701__1" + "metadata__9223372036438563903__9223372036854775798__" + md3Timestamp + "__31__9223372036854775701__1" ); List metadataFilesToBeDeleted = RemoteFsTimestampAwareTranslog.getMetadataFilesToBeDeleted(