Incorporating PR feedback from @mch2

Signed-off-by: Kartik Ganesh <[email protected]>

Author: kartg

server/src/main/java/org/opensearch/index/IndexSettings.java

@@ -756,6 +756,8 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti
 
         if (isRemoteSnapshot && FeatureFlags.isEnabled(SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY)) {
             extendedCompatibilitySnapshotVersion = SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION;
+        } else {
+            extendedCompatibilitySnapshotVersion = Version.CURRENT.minimumIndexCompatibilityVersion();
         }
         this.searchThrottled = INDEX_SEARCH_THROTTLED.get(settings);
         this.queryStringLenient = QUERY_STRING_LENIENT_SETTING.get(settings);

server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java

@@ -97,28 +97,6 @@ public class ReadOnlyEngine extends Engine {
 
     protected volatile TranslogStats translogStats;
 
-    /**
-     * Wrapper constructor that turns off support for reading old indices.
-     */
-    public ReadOnlyEngine(
-        EngineConfig config,
-        SeqNoStats seqNoStats,
-        TranslogStats translogStats,
-        boolean obtainLock,
-        Function<DirectoryReader, DirectoryReader> readerWrapperFunction,
-        boolean requireCompleteHistory
-    ) {
-        this(
-            config,
-            seqNoStats,
-            translogStats,
-            obtainLock,
-            readerWrapperFunction,
-            requireCompleteHistory,
-            Version.CURRENT.minimumIndexCompatibilityVersion()
-        );
-    }
-
     /**
      * Creates a new ReadOnlyEngine. This ctor can also be used to open a read-only engine on top of an already opened
      * read-write engine. It allows to optionally obtain the writer locks for the shard which would time-out if another
@@ -131,20 +109,19 @@ public ReadOnlyEngine(
      *                   the lock won't be obtained
      * @param readerWrapperFunction allows to wrap the index-reader for this engine.
      * @param requireCompleteHistory indicates whether this engine permits an incomplete history (i.e. LCP &lt; MSN)
-     * @param minimumSupportedVersion for extended backward compatibility use-cases, the minimum {@link Version} this engine is allowed to read
      */
     public ReadOnlyEngine(
         EngineConfig config,
         SeqNoStats seqNoStats,
         TranslogStats translogStats,
         boolean obtainLock,
         Function<DirectoryReader, DirectoryReader> readerWrapperFunction,
-        boolean requireCompleteHistory,
-        Version minimumSupportedVersion
+        boolean requireCompleteHistory
     ) {
         super(config);
         this.requireCompleteHistory = requireCompleteHistory;
-        this.minimumSupportedVersion = minimumSupportedVersion;
+        // fetch the minimum Version for extended backward compatibility use-cases
+        this.minimumSupportedVersion = config.getIndexSettings().getExtendedCompatibilitySnapshotVersion();
         try {
             Store store = config.getStore();
             store.incRef();
@@ -263,7 +240,7 @@ protected DirectoryReader open(IndexCommit commit) throws IOException {
     }
 
     private boolean isExtendedCompatibility() {
-        return this.minimumSupportedVersion.before(Version.CURRENT.minimumIndexCompatibilityVersion());
+        return Version.CURRENT.minimumIndexCompatibilityVersion().onOrAfter(this.minimumSupportedVersion);
     }
 
     @Override

server/src/main/java/org/opensearch/indices/IndicesService.java

@@ -888,15 +888,7 @@ private EngineFactory getEngineFactory(final IndexSettings idxSettings) {
                 return new NRTReplicationEngineFactory();
             }
             if (idxSettings.isRemoteSnapshot()) {
-                return config -> new ReadOnlyEngine(
-                    config,
-                    new SeqNoStats(0, 0, 0),
-                    new TranslogStats(),
-                    true,
-                    Function.identity(),
-                    false,
-                    idxSettings.getExtendedCompatibilitySnapshotVersion()
-                );
+                return config -> new ReadOnlyEngine(config, new SeqNoStats(0, 0, 0), new TranslogStats(), true, Function.identity(), false);
             }
             return new InternalEngineFactory();
         } else if (engineFactories.size() == 1) {

server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java

@@ -32,21 +32,29 @@
 package org.opensearch.index.engine;
 
 import org.apache.lucene.index.DirectoryReader;
+import org.apache.lucene.index.IndexFormatTooOldException;
 import org.apache.lucene.index.IndexReader;
 import org.apache.lucene.tests.util.LuceneTestCase;
 import org.apache.lucene.tests.util.TestUtil;
-import org.opensearch.LegacyESVersion;
 import org.opensearch.Version;
+import org.opensearch.cluster.metadata.IndexMetadata;
 import org.opensearch.common.bytes.BytesArray;
 import org.opensearch.common.lucene.index.OpenSearchDirectoryReader;
+import org.opensearch.common.settings.Settings;
+import org.opensearch.common.util.FeatureFlags;
 import org.opensearch.core.internal.io.IOUtils;
+import org.opensearch.index.IndexModule;
+import org.opensearch.index.IndexSettings;
 import org.opensearch.index.mapper.ParsedDocument;
 import org.opensearch.index.seqno.SeqNoStats;
 import org.opensearch.index.seqno.SequenceNumbers;
 import org.opensearch.index.store.Store;
 import org.opensearch.index.translog.TranslogStats;
+import org.opensearch.test.FeatureFlagSetter;
+import org.opensearch.test.IndexSettingsModule;
 
 import java.io.IOException;
+import java.io.UncheckedIOException;
 import java.nio.file.Path;
 import java.util.List;
 import java.util.concurrent.atomic.AtomicLong;
@@ -231,7 +239,33 @@ public void testReadOnly() throws IOException {
         }
     }
 
-    public void testReadOldIndices() throws IOException {
+    public void testReadOldIndices() throws Exception {
+        IOUtils.close(engine, store);
+        // The index has one document in it, so the checkpoint cannot be NO_OPS_PERFORMED
+        final AtomicLong globalCheckpoint = new AtomicLong(0);
+        final String pathToTestIndex = "/indices/bwc/es-6.3.0/testIndex-es-6.3.0.zip";
+        Path tmp = createTempDir();
+        TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp);
+        try (FeatureFlagSetter f = FeatureFlagSetter.set(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY)) {
+            final IndexSettings indexSettings = IndexSettingsModule.newIndexSettings(
+                "index",
+                Settings.builder()
+                    .put(IndexMetadata.SETTING_VERSION_CREATED, org.opensearch.Version.CURRENT)
+                    .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey())
+                    .build()
+            );
+            try (Store store = createStore(newFSDirectory(tmp))) {
+                EngineConfig config = config(indexSettings, store, createTempDir(), newMergePolicy(), null, null, globalCheckpoint::get);
+                try (
+                    ReadOnlyEngine readOnlyEngine = new ReadOnlyEngine(config, null, new TranslogStats(), true, Function.identity(), true)
+                ) {
+                    assertVisibleCount(readOnlyEngine, 1, false);
+                }
+            }
+        }
+    }
+
+    public void testReadOldIndicesFailure() throws IOException {
         IOUtils.close(engine, store);
         // The index has one document in it, so the checkpoint cannot be NO_OPS_PERFORMED
         final AtomicLong globalCheckpoint = new AtomicLong(0);
@@ -240,18 +274,10 @@ public void testReadOldIndices() throws IOException {
         TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp);
         try (Store store = createStore(newFSDirectory(tmp))) {
             EngineConfig config = config(defaultSettings, store, createTempDir(), newMergePolicy(), null, null, globalCheckpoint::get);
-            try (
-                ReadOnlyEngine readOnlyEngine = new ReadOnlyEngine(
-                    config,
-                    null,
-                    new TranslogStats(),
-                    true,
-                    Function.identity(),
-                    true,
-                    LegacyESVersion.fromId(6000099)
-                )
-            ) {
-                assertVisibleCount(readOnlyEngine, 1, false);
+            try {
+                new ReadOnlyEngine(config, null, new TranslogStats(), true, Function.identity(), true);
+            } catch (UncheckedIOException e) {
+                assertEquals(IndexFormatTooOldException.class, e.getCause().getClass());
             }
         }
     }