Incorporating PR feedback

Signed-off-by: Kartik Ganesh <gkart@amazon.com>
opensearch-project · Dec 9, 2022 · 7531e14 · 7531e14
1 parent ff448f4
commit 7531e14
Show file tree

Hide file tree

Showing 9 changed files with 98 additions and 29 deletions.
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
@@ -768,26 +768,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"
+}
+'
+```