Addressed CR Comments

opensearch-project · Sep 6, 2021 · acd09db · acd09db
1 parent 00c892d
commit acd09db
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 7 deletions.
diff --git a/server/src/main/java/org/opensearch/index/IndexSettings.java b/server/src/main/java/org/opensearch/index/IndexSettings.java
@@ -297,8 +297,7 @@ public final class IndexSettings {
 
     public static final Setting<ByteSizeValue> INDEX_TRANSLOG_RETENTION_SIZE_SETTING =
         Setting.byteSizeSetting("index.translog.retention.size",
-            settings -> shouldDisableTranslogRetention(settings) &&
-                !shouldPruneTranslogByRetentionLease(settings) ? "-1" : DEFAULT_TRANSLOG_RETENTION_SIZE.getStringRep(),
+            settings -> DEFAULT_TRANSLOG_RETENTION_SIZE.getStringRep(),
             Property.Dynamic, Property.IndexScope);
 
     /**
@@ -536,7 +535,8 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti
         mergeSchedulerConfig = new MergeSchedulerConfig(this);
         gcDeletesInMillis = scopedSettings.get(INDEX_GC_DELETES_SETTING).getMillis();
         softDeleteEnabled = version.onOrAfter(LegacyESVersion.V_6_5_0) && scopedSettings.get(INDEX_SOFT_DELETES_SETTING);
-        translogPruningByRetentionLease = version.onOrAfter(LegacyESVersion.V_6_5_0) &&
+        translogPruningByRetentionLease = version.onOrAfter(Version.V_1_1_0) &&
+            scopedSettings.get(INDEX_SOFT_DELETES_SETTING) &&
             scopedSettings.get(INDEX_TRANSLOG_RETENTION_LEASE_PRUNING_ENABLED_SETTING);
         softDeleteRetentionOperations = scopedSettings.get(INDEX_SOFT_DELETES_RETENTION_OPERATIONS_SETTING);
         retentionLeaseMillis = scopedSettings.get(INDEX_SOFT_DELETES_RETENTION_LEASE_PERIOD_SETTING).millis();
@@ -639,8 +639,8 @@ private void setFlushAfterMergeThresholdSize(ByteSizeValue byteSizeValue) {
     }
 
     private void setTranslogPruningByRetentionLease(boolean enabled) {
-        this.translogPruningByRetentionLease = enabled;
-        if(enabled) {
+        this.translogPruningByRetentionLease = this.softDeleteEnabled && enabled;
+        if(translogPruningByRetentionLease) {
             setTranslogRetentionSize(DEFAULT_TRANSLOG_RETENTION_SIZE);
         }
     }
@@ -850,8 +850,12 @@ public TimeValue getRefreshInterval() {
      * Returns the transaction log retention size which controls how much of the translog is kept around to allow for ops based recoveries
      */
     public ByteSizeValue getTranslogRetentionSize() {
-        assert shouldDisableTranslogRetention(settings) && !shouldPruneTranslogByRetentionLease() == false ||
-            translogRetentionSize.getBytes() == -1L : translogRetentionSize;
+        if(shouldDisableTranslogRetention(settings) && !shouldPruneTranslogByRetentionLease(settings)) {
+            return new ByteSizeValue(-1);
+        }
+        else if(shouldPruneTranslogByRetentionLease(settings) && translogRetentionSize.getBytes() == -1) {
+            return DEFAULT_TRANSLOG_RETENTION_SIZE;
+        }
         return translogRetentionSize;
     }
 

diff --git a/server/src/main/java/org/opensearch/index/shard/IndexShard.java b/server/src/main/java/org/opensearch/index/shard/IndexShard.java
@@ -1959,6 +1959,8 @@ public void onSettingsChanged() {
         Engine engineOrNull = getEngineOrNull();
         if (engineOrNull != null) {
             final boolean disableTranslogRetention = indexSettings.isSoftDeleteEnabled() && useRetentionLeasesInPeerRecovery;
+            logger.error("testIndex - shouldPruneTranslogByRetentionLease = {}", indexSettings.shouldPruneTranslogByRetentionLease());
+            logger.error("testIndex - retentionSizeInBytes = {}", indexSettings.getTranslogRetentionSize());
             engineOrNull.onSettingsChanged(
                 disableTranslogRetention ? TimeValue.MINUS_ONE : indexSettings.getTranslogRetentionAge(),
                 disableTranslogRetention && !indexSettings.shouldPruneTranslogByRetentionLease() ?

diff --git a/server/src/main/java/org/opensearch/index/translog/TranslogDeletionPolicy.java b/server/src/main/java/org/opensearch/index/translog/TranslogDeletionPolicy.java
@@ -32,9 +32,11 @@
 
 package org.opensearch.index.translog;
 
+import org.apache.logging.log4j.Logger;
 import org.apache.lucene.util.Counter;
 import org.opensearch.Assertions;
 import org.opensearch.common.lease.Releasable;
+import org.opensearch.common.logging.Loggers;
 import org.opensearch.index.seqno.RetentionLease;
 import org.opensearch.index.seqno.RetentionLeases;
 import org.opensearch.index.seqno.SequenceNumbers;
@@ -51,6 +53,7 @@ public class TranslogDeletionPolicy {
 
     private final Map<Object, RuntimeException> openTranslogRef;
     private Supplier<RetentionLeases> retentionLeasesSupplier;
+    private static Logger log = Loggers.getLogger(TranslogDeletionPolicy.class, "TestTranslog");
 
     public void assertNoOpenTranslogRefs() {
         if (openTranslogRef.isEmpty() == false) {
@@ -114,6 +117,8 @@ synchronized void setRetentionTotalFiles(int retentionTotalFiles) {
 
     public synchronized void shouldPruneTranslogByRetentionLease(boolean translogPruneByRetentionLease) {
         this.shouldPruneTranslogByRetentionLease = translogPruneByRetentionLease;
+        log.error("testIndex - shouldPruneTranslogByRetentionLease = {}", shouldPruneTranslogByRetentionLease);
+        log.error("testIndex - retentionSizeInBytes = {}", retentionSizeInBytes);
     }
 
     /**

diff --git a/server/src/test/java/org/opensearch/index/translog/TranslogDeletionPolicyTests.java b/server/src/test/java/org/opensearch/index/translog/TranslogDeletionPolicyTests.java
@@ -90,6 +90,11 @@ public void testBytesRetention() throws IOException {
         }
     }
 
+    public void testBytesRetentionWithRetentionLease() throws Exception {
+        long now = System.currentTimeMillis();
+
+    }
+
     public void testAgeRetention() throws IOException {
         long now = System.currentTimeMillis();
         Tuple<List<TranslogReader>, TranslogWriter> readersAndWriter = createReadersAndWriter(now);