Change the way we skip calling list metadata repeatedly

Signed-off-by: Sachin Kale <[email protected]>
sachinpkale · Sep 19, 2024 · b02703a · b02703a
1 parent ded793d
commit b02703a
Show file tree

Hide file tree

Showing 3 changed files with 11 additions and 67 deletions.
diff --git a/...rTest/java/org/opensearch/remotestore/RemoteStorePinnedTimestampsGarbageCollectionIT.java b/...rTest/java/org/opensearch/remotestore/RemoteStorePinnedTimestampsGarbageCollectionIT.java
@@ -118,17 +118,15 @@ public void testLiveIndexNoPinnedTimestamps() throws Exception {
         });
     }
 
-    public void testLiveIndexNoPinnedTimestampsWithExtraGenSettingWithinLimit() throws Exception {
+    public void testLiveIndexNoPinnedTimestampsWithMetadataSkippedOnLastDeletionCheck() throws Exception {
         prepareCluster(1, 1, Settings.EMPTY);
-        Settings indexSettings = Settings.builder()
-            .put(remoteStoreIndexSettings(0, 1))
-            .put(INDEX_REMOTE_TRANSLOG_KEEP_EXTRA_GEN_SETTING.getKey(), 10)
-            .build();
+        Settings indexSettings = Settings.builder().put(remoteStoreIndexSettings(0, 1)).build();
         createIndex(INDEX_NAME, indexSettings);
         ensureYellowAndNoInitializingShards(INDEX_NAME);
         ensureGreen(INDEX_NAME);
 
-        RemoteStoreSettings.setPinnedTimestampsLookbackInterval(TimeValue.ZERO);
+        // We don't set look-back interval to 0 as we want GC to skip based on last deletion check
+        // RemoteStoreSettings.setPinnedTimestampsLookbackInterval(TimeValue.ZERO);
 
         RemoteStorePinnedTimestampService remoteStorePinnedTimestampService = internalCluster().getInstance(
             RemoteStorePinnedTimestampService.class,
@@ -173,61 +171,6 @@ public void testLiveIndexNoPinnedTimestampsWithExtraGenSettingWithinLimit() thro
         });
     }
 
-    public void testLiveIndexNoPinnedTimestampsWithExtraGenSetting() throws Exception {
-        prepareCluster(1, 1, Settings.EMPTY);
-        Settings indexSettings = Settings.builder()
-            .put(remoteStoreIndexSettings(0, 1))
-            .put(INDEX_REMOTE_TRANSLOG_KEEP_EXTRA_GEN_SETTING.getKey(), 3)
-            .build();
-        createIndex(INDEX_NAME, indexSettings);
-        ensureYellowAndNoInitializingShards(INDEX_NAME);
-        ensureGreen(INDEX_NAME);
-
-        RemoteStoreSettings.setPinnedTimestampsLookbackInterval(TimeValue.ZERO);
-
-        RemoteStorePinnedTimestampService remoteStorePinnedTimestampService = internalCluster().getInstance(
-            RemoteStorePinnedTimestampService.class,
-            primaryNodeName(INDEX_NAME)
-        );
-
-        remoteStorePinnedTimestampService.rescheduleAsyncUpdatePinnedTimestampTask(TimeValue.timeValueSeconds(1));
-
-        int numDocs = 5;
-        for (int i = 0; i < numDocs; i++) {
-            keepPinnedTimestampSchedulerUpdated();
-            indexSingleDoc(INDEX_NAME, true);
-        }
-
-        String translogPathFixedPrefix = RemoteStoreSettings.CLUSTER_REMOTE_STORE_TRANSLOG_PATH_PREFIX.get(getNodeSettings());
-        String shardDataPath = getShardLevelBlobPath(
-            client(),
-            INDEX_NAME,
-            BlobPath.cleanPath(),
-            "0",
-            TRANSLOG,
-            DATA,
-            translogPathFixedPrefix
-        ).buildAsString();
-        Path translogDataPath = Path.of(translogRepoPath + "/" + shardDataPath + "/1");
-        String shardMetadataPath = getShardLevelBlobPath(
-            client(),
-            INDEX_NAME,
-            BlobPath.cleanPath(),
-            "0",
-            TRANSLOG,
-            METADATA,
-            translogPathFixedPrefix
-        ).buildAsString();
-        Path translogMetadataPath = Path.of(translogRepoPath + "/" + shardMetadataPath);
-
-        assertBusy(() -> {
-            List<Path> metadataFiles = Files.list(translogMetadataPath).collect(Collectors.toList());
-            assertEquals(3, metadataFiles.size());
-
-            verifyTranslogDataFileCount(metadataFiles, translogDataPath);
-        });
-    }
-
     public void testLiveIndexWithPinnedTimestamps() throws Exception {
         prepareCluster(1, 1, Settings.EMPTY);
         Settings indexSettings = Settings.builder()

diff --git a/server/src/main/java/org/opensearch/index/translog/RemoteFsTimestampAwareTranslog.java b/server/src/main/java/org/opensearch/index/translog/RemoteFsTimestampAwareTranslog.java
@@ -61,7 +61,7 @@ public class RemoteFsTimestampAwareTranslog extends RemoteFsTranslog {
     private final Map<String, Tuple<Long, Long>> oldFormatMetadataFileGenerationMap;
     private final Map<String, Tuple<Long, Long>> oldFormatMetadataFilePrimaryTermMap;
     private final AtomicLong minPrimaryTermInRemote = new AtomicLong(Long.MAX_VALUE);
-    private long maxDeletedGenerationOnRemote = 0;
+    private long lastTimestampOfMetadataDeletionOnRemote = System.currentTimeMillis();
 
     public RemoteFsTimestampAwareTranslog(
         TranslogConfig config,
@@ -148,8 +148,10 @@ protected void trimUnreferencedReaders(boolean indexDeleted, boolean trimLocal)
 
         // This code block ensures parity with RemoteFsTranslog. Without this, we will end up making list translog metadata
         // call in each invocation of trimUnreferencedReaders
-        long minGenerationToKeep = minRemoteGenReferenced - indexSettings().getRemoteTranslogExtraKeep();
-        if (indexDeleted == false && (minGenerationToKeep <= maxDeletedGenerationOnRemote)) {
+        if (indexDeleted == false
+            && (System.currentTimeMillis() - lastTimestampOfMetadataDeletionOnRemote <= RemoteStoreSettings
+                .getPinnedTimestampsLookbackInterval()
+                .millis() * 2)) {
             return;
         }
 
@@ -207,8 +209,6 @@ public void onResponse(List<BlobMetadata> blobMetadata) {
 
                     logger.debug(() -> "generationsToBeDeleted = " + generationsToBeDeleted);
                     if (generationsToBeDeleted.isEmpty() == false) {
-                        maxDeletedGenerationOnRemote = generationsToBeDeleted.stream().max(Long::compareTo).get();
-
                         // Delete stale generations
                         translogTransferManager.deleteGenerationAsync(
                             primaryTermSupplier.getAsLong(),
@@ -220,6 +220,7 @@ public void onResponse(List<BlobMetadata> blobMetadata) {
                     }
 
                     if (metadataFilesToBeDeleted.isEmpty() == false) {
+                        lastTimestampOfMetadataDeletionOnRemote = System.currentTimeMillis();
                         // Delete stale metadata files
                         translogTransferManager.deleteMetadataFilesAsync(
                             metadataFilesToBeDeleted,

diff --git a/server/src/test/java/org/opensearch/index/translog/RemoteFsTimestampAwareTranslogTests.java b/server/src/test/java/org/opensearch/index/translog/RemoteFsTimestampAwareTranslogTests.java
@@ -783,7 +783,7 @@ public void testGetMetadataFilesToBeDeletedExclusionBasedOnPinningOnly() throws
 
     public void testGetMetadataFilesToBeDeletedExclusionBasedOnAgeAndPinning() throws IOException {
         long currentTimeInMillis = System.currentTimeMillis();
-        String md1Timestamp = RemoteStoreUtils.invertLong(currentTimeInMillis - 200000);
+        String md1Timestamp = RemoteStoreUtils.invertLong(currentTimeInMillis + 100000);
         String md2Timestamp = RemoteStoreUtils.invertLong(currentTimeInMillis - 300000);
         String md3Timestamp = RemoteStoreUtils.invertLong(currentTimeInMillis - 600000);