Incorporating PR feedback

kartg · kartg · commit 78681fbfaf3e · 2022-12-15T10:39:35.000-08:00
Signed-off-by: Kartik Ganesh &lt;gkart@amazon.com&gt;
diff --git a/server/src/main/java/org/opensearch/common/lucene/Lucene.java b/server/src/main/java/org/opensearch/common/lucene/Lucene.java
@@ -171,7 +171,7 @@ public static SegmentInfos readSegmentInfos(Directory directory) throws IOExcept
      * the latest index commit is opened via {@link DirectoryReader} by including a minimum supported
      * Lucene major version based on the minimum compatibility of the given {@link org.opensearch.Version}.
      */
-    public static SegmentInfos readSegmentInfosExtendedCompatbility(Directory directory, org.opensearch.Version minimumVersion)
+    public static SegmentInfos readSegmentInfosExtendedCompatibility(Directory directory, org.opensearch.Version minimumVersion)
         throws IOException {
         // This list is sorted from oldest to latest
         List<IndexCommit> indexCommits = DirectoryReader.listCommits(directory);
diff --git a/server/src/main/java/org/opensearch/index/IndexSettings.java b/server/src/main/java/org/opensearch/index/IndexSettings.java
@@ -758,8 +758,8 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti
         isRemoteTranslogStoreEnabled = settings.getAsBoolean(IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_ENABLED, false);
         isRemoteSnapshot = IndexModule.Type.REMOTE_SNAPSHOT.match(this);
         if (isRemoteSnapshot) {
-            Integer minVersion = settings.getAsInt(SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.getKey(), 0);
-            if (minVersion > 0) {
+            Integer minVersion = settings.getAsInt(SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.getKey(), Version.V_EMPTY_ID);
+            if (minVersion > Version.V_EMPTY_ID) {
                 extendedCompatibilitySnapshotVersion = LegacyESVersion.fromId(minVersion);
             }
         }
diff --git a/server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java b/server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java
@@ -151,7 +151,7 @@ public ReadOnlyEngine(
                 if (this.minimumSupportedVersion == null) {
                     this.lastCommittedSegmentInfos = Lucene.readSegmentInfos(directory);
                 } else {
-                    this.lastCommittedSegmentInfos = Lucene.readSegmentInfosExtendedCompatbility(directory, this.minimumSupportedVersion);
+                    this.lastCommittedSegmentInfos = Lucene.readSegmentInfosExtendedCompatibility(directory, this.minimumSupportedVersion);
                 }
                 if (seqNoStats == null) {
                     seqNoStats = buildSeqNoStats(config, lastCommittedSegmentInfos);
diff --git a/server/src/main/java/org/opensearch/index/shard/IndexShard.java b/server/src/main/java/org/opensearch/index/shard/IndexShard.java
@@ -2042,7 +2042,7 @@ private boolean assertSequenceNumbersInCommit() throws IOException {
         final Map<String, String> userData;
         if (indexSettings.isRemoteSnapshot() && indexSettings.getExtendedCompatibilitySnapshotVersion() != null) {
             // Inefficient method to support reading old Lucene indexes
-            userData = Lucene.readSegmentInfosExtendedCompatbility(
+            userData = Lucene.readSegmentInfosExtendedCompatibility(
                 store.directory(),
                 indexSettings.getExtendedCompatibilitySnapshotVersion()
             ).getUserData();
diff --git a/server/src/main/java/org/opensearch/index/store/Store.java b/server/src/main/java/org/opensearch/index/store/Store.java
@@ -263,7 +263,7 @@ private static SegmentInfos readSegmentsInfo(IndexCommit commit, Directory direc
     private static SegmentInfos readSegmentInfosExtendedCompatbility(Directory directory, org.opensearch.Version minimumVersion)
         throws IOException {
         try {
-            return Lucene.readSegmentInfosExtendedCompatbility(directory, minimumVersion);
+            return Lucene.readSegmentInfosExtendedCompatibility(directory, minimumVersion);
         } catch (EOFException eof) {
             // TODO this should be caught by lucene - EOF is almost certainly an index corruption
             throw new CorruptIndexException("Read past EOF while reading segment infos", "<latest-commit>", eof);
diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java
@@ -888,26 +888,15 @@ private EngineFactory getEngineFactory(final IndexSettings idxSettings) {
                 return new NRTReplicationEngineFactory();
             }
             if (idxSettings.isRemoteSnapshot()) {
-                if (idxSettings.getExtendedCompatibilitySnapshotVersion() != null) {
-                    return config -> new ReadOnlyEngine(
-                        config,
-                        new SeqNoStats(0, 0, 0),
-                        new TranslogStats(),
-                        true,
-                        Function.identity(),
-                        false,
-                        idxSettings.getExtendedCompatibilitySnapshotVersion()
-                    );
-                } else {
-                    return config -> new ReadOnlyEngine(
-                        config,
-                        new SeqNoStats(0, 0, 0),
-                        new TranslogStats(),
-                        true,
-                        Function.identity(),
-                        false
-                    );
-                }
+                return config -> new ReadOnlyEngine(
+                    config,
+                    new SeqNoStats(0, 0, 0),
+                    new TranslogStats(),
+                    true,
+                    Function.identity(),
+                    false,
+                    idxSettings.getExtendedCompatibilitySnapshotVersion()
+                );
             }
             return new InternalEngineFactory();
         } else if (engineFactories.size() == 1) {
diff --git a/server/src/test/java/org/opensearch/common/lucene/LuceneTests.java b/server/src/test/java/org/opensearch/common/lucene/LuceneTests.java
@@ -31,7 +31,10 @@
 
 package org.opensearch.common.lucene;
 
+import org.apache.lucene.document.LatLonPoint;
+import org.apache.lucene.index.IndexCommit;
 import org.apache.lucene.index.IndexFormatTooOldException;
+import org.apache.lucene.index.StandardDirectoryReader;
 import org.apache.lucene.tests.analysis.MockAnalyzer;
 import org.apache.lucene.analysis.core.KeywordAnalyzer;
 import org.apache.lucene.document.Document;
@@ -76,6 +79,7 @@
 import org.apache.lucene.util.Bits;
 import org.apache.lucene.util.BytesRef;
 import org.opensearch.LegacyESVersion;
+import org.opensearch.Version;
 import org.opensearch.common.collect.Tuple;
 import org.opensearch.common.io.stream.NamedWriteableRegistry;
 import org.opensearch.core.internal.io.IOUtils;
@@ -372,15 +376,34 @@ public void testNumDocs() throws IOException {
         dir.close();
     }
 
-    public void testReadAnySegmentInfos() throws IOException {
+    /**
+     * Tests whether old segments are readable and queryable based on the data documented
+     * in the README <a href="file:../../../../../resources/indices/bwc/testIndex-6.3.0.md">here</a>.
+     *
+     * @throws IOException
+     */
+    public void testReadSegmentInfosExtendedCompatibility() throws IOException {
         final String pathToTestIndex = "/indices/bwc/testIndex-6.3.0.zip";
+        final Version minVersion = LegacyESVersion.fromId(6000099);
         Path tmp = createTempDir();
         TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp);
         MockDirectoryWrapper dir = newMockFSDirectory(tmp);
         // The standard API will throw an exception
         expectThrows(IndexFormatTooOldException.class, () -> Lucene.readSegmentInfos(dir));
-        SegmentInfos si = Lucene.readSegmentInfosExtendedCompatbility(dir, LegacyESVersion.fromId(6000099));
+        SegmentInfos si = Lucene.readSegmentInfosExtendedCompatibility(dir, minVersion);
         assertEquals(1, Lucene.getNumDocs(si));
+        IndexCommit indexCommit = Lucene.getIndexCommit(si, dir);
+        // uses the "expert" Lucene API
+        StandardDirectoryReader reader = (StandardDirectoryReader) DirectoryReader.open(
+            indexCommit,
+            minVersion.minimumIndexCompatibilityVersion().luceneVersion.major,
+            null
+        );
+        IndexSearcher searcher = newSearcher(reader);
+        // radius too small, should get no results
+        assertFalse(Lucene.exists(searcher, LatLonPoint.newDistanceQuery("testLocation", 48.57532, -112.87695, 2)));
+        assertTrue(Lucene.exists(searcher, LatLonPoint.newDistanceQuery("testLocation", 48.57532, -112.87695, 20000)));
+        reader.close();
         dir.close();
     }
 
diff --git a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java
@@ -961,7 +961,7 @@ public void testExtendedCompatibilityVersionForRemoteSnapshot() {
         assertEquals(expected, settings.getExtendedCompatibilitySnapshotVersion());
     }
 
-    public void testExtendedCompatibilityVersionForNonSnapshot() {
+    public void testExtendedCompatibilityVersionForNonRemoteSnapshot() {
         IndexMetadata metadata = newIndexMeta(
             "index",
             Settings.builder()
diff --git a/server/src/test/resources/indices/bwc/testIndex-6.3.0.md b/server/src/test/resources/indices/bwc/testIndex-6.3.0.md
@@ -0,0 +1,57 @@
+# README for _testIndex-6.3.0.zip_
+
+This zip file holds a Lucene index created using ElasticSearch 6.3.0.
+It was created by running the underlying commands against a single-node cluster,
+then compressing the contents of the underlying Lucene index directory i.e.
+the files under `<elasticsearch-root>/data/nodes/0/indices/<index-uuid>/0/index`.
+The index contains one document.
+
+## Commands
+
+```
+curl -X PUT -H 'Content-Type: application/json' 'localhost:9200/testindex?pretty' -d'
+{
+  "settings": {
+    "number_of_shards": 1,
+    "number_of_replicas": 0
+  },
+  "mappings": {
+    "testData": {
+      "properties": {
+        "id": { "type": "keyword" },
+        "isTestData": { "type": "boolean" },
+        "testNum": { "type": "short" },
+        "testRange": {"type": "integer_range" },
+        "testMessage": {
+          "type": "text",
+          "fields": {
+            "length": {
+              "type": "token_count",
+              "analyzer": "standard"
+            }
+          }
+        },
+        "testBlob": { "type": "binary", "index": false },
+        "testDate": { "type": "date" },
+        "testLocation": { "type": "geo_point"}
+      }
+    }
+  }
+}'
+
+curl -X POST "localhost:9200/testindex/testData/?pretty" -H 'Content-Type: application/json' -d'
+{
+  "id": "testData1",
+  "isTestData": true,
+  "testNum": 99,
+  "testRange": {
+    "gte": 0,
+    "lte": 100
+  },
+  "testMessage": "The OpenSearch Project",
+  "testBlob": "VGhlIE9wZW5TZWFyY2ggUHJvamVjdA==",
+  "testDate": "1970-01-02",
+  "testLocation": "48.553532,-113.022881"
+}
+'
+```

Original file line number	Diff line number	Diff line change
`@@ -758,8 +758,8 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti`
`758`	`758`	`isRemoteTranslogStoreEnabled = settings.getAsBoolean(IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_ENABLED, false);`
`759`	`759`	`isRemoteSnapshot = IndexModule.Type.REMOTE_SNAPSHOT.match(this);`
`760`	`760`	`if (isRemoteSnapshot) {`
`761`		`- Integer minVersion = settings.getAsInt(SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.getKey(), 0);`
`762`		`- if (minVersion > 0) {`
	`761`	`+ Integer minVersion = settings.getAsInt(SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.getKey(), Version.V_EMPTY_ID);`
	`762`	`+ if (minVersion > Version.V_EMPTY_ID) {`
`763`	`763`	`extendedCompatibilitySnapshotVersion = LegacyESVersion.fromId(minVersion);`
`764`	`764`	`}`
`765`	`765`	`}`
Original file line number	Diff line number	Diff line change
`@@ -151,7 +151,7 @@ public ReadOnlyEngine(`
`151`	`151`	`if (this.minimumSupportedVersion == null) {`
`152`	`152`	`this.lastCommittedSegmentInfos = Lucene.readSegmentInfos(directory);`
`153`	`153`	`} else {`
`154`		`- this.lastCommittedSegmentInfos = Lucene.readSegmentInfosExtendedCompatbility(directory, this.minimumSupportedVersion);`
	`154`	`+ this.lastCommittedSegmentInfos = Lucene.readSegmentInfosExtendedCompatibility(directory, this.minimumSupportedVersion);`
`155`	`155`	`}`
`156`	`156`	`if (seqNoStats == null) {`
`157`	`157`	`seqNoStats = buildSeqNoStats(config, lastCommittedSegmentInfos);`
Original file line number	Diff line number	Diff line change
`@@ -961,7 +961,7 @@ public void testExtendedCompatibilityVersionForRemoteSnapshot() {`
`961`	`961`	`assertEquals(expected, settings.getExtendedCompatibilitySnapshotVersion());`
`962`	`962`	`}`
`963`	`963`
`964`		`- public void testExtendedCompatibilityVersionForNonSnapshot() {`
	`964`	`+ public void testExtendedCompatibilityVersionForNonRemoteSnapshot() {`
`965`	`965`	`IndexMetadata metadata = newIndexMeta(`
`966`	`966`	`"index",`
`967`	`967`	`Settings.builder()`