From 5d4acf6f163a7c211871bc345899d1cc1d9744fd Mon Sep 17 00:00:00 2001 From: Kartik Ganesh Date: Mon, 14 Nov 2022 14:02:12 -0800 Subject: [PATCH 01/10] Enhance searchable snapshots to enable a read-only view of older snapshots This change removes the guardrails around N-1 backward compatibility and uses Lucene's "expert" APIs to read snapshots (Lucene segments) older than N-1 on a best-effort basis. The functionality is gated by an additional feature flag, separate from the searchable snapshots flag. Note that the Lucene integration is rather inefficient because the necessary "expert" Lucene APIs are still package-private. Signed-off-by: Kartik Ganesh --- .../src/main/java/org/opensearch/Version.java | 6 ++++ .../cluster/metadata/IndexMetadata.java | 7 ++-- .../org/opensearch/common/lucene/Lucene.java | 18 ++++++++++ .../opensearch/common/util/FeatureFlags.java | 7 ++++ .../index/engine/ReadOnlyEngine.java | 34 +++++++++++++++++-- .../opensearch/index/shard/IndexShard.java | 15 ++++++-- .../org/opensearch/index/store/Store.java | 32 +++++++++++++++-- .../opensearch/indices/IndicesService.java | 22 +++++++++++- .../blobstore/FileRestoreContext.java | 5 +++ .../java/org/opensearch/VersionTests.java | 21 ++++++++++++ 10 files changed, 157 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/org/opensearch/Version.java b/server/src/main/java/org/opensearch/Version.java index 6c105a6233126..9162f84fbea8f 100644 --- a/server/src/main/java/org/opensearch/Version.java +++ b/server/src/main/java/org/opensearch/Version.java @@ -38,6 +38,7 @@ import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.io.stream.StreamOutput; import org.opensearch.common.settings.Settings; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.xcontent.ToXContentFragment; import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.monitor.jvm.JvmInfo; @@ -51,6 +52,8 @@ import java.util.Locale; import java.util.Objects; +import static org.opensearch.common.util.FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC; + /** * OpenSearch Version Class * @@ -415,6 +418,9 @@ private Version computeMinIndexCompatVersion() { } else if (major == 7 || major == 1) { return LegacyESVersion.fromId(6000026); } else if (major == 2) { + if (FeatureFlags.isEnabled(SEARCHABLE_SNAPSHOT_EXTENDED_BWC)) { + return LegacyESVersion.fromId(6030099); + } return LegacyESVersion.V_7_0_0; } else { bwcMajor = major - 1; diff --git a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java index ae135e1ad4ff3..71f2666c2371c 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java @@ -1825,10 +1825,13 @@ public static IndexMetadata fromXContent(XContentParser parser) throws IOExcepti throw new IllegalArgumentException("Unexpected token " + token); } } - if (Assertions.ENABLED) { + // Reference: + // https://github.com/opensearch-project/OpenSearch/blob/4dde0f2a3b445b2fc61dab29c5a2178967f4a3e3/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java#L1620-L1628 + Version legacyVersion = LegacyESVersion.fromId(6050099); + if (Assertions.ENABLED && Version.indexCreated(builder.settings).onOrAfter(legacyVersion)) { assert mappingVersion : "mapping version should be present for indices"; } - if (Assertions.ENABLED) { + if (Assertions.ENABLED && Version.indexCreated(builder.settings).onOrAfter(legacyVersion)) { assert settingsVersion : "settings version should be present for indices"; } if (Assertions.ENABLED && Version.indexCreated(builder.settings).onOrAfter(LegacyESVersion.V_7_2_0)) { diff --git a/server/src/main/java/org/opensearch/common/lucene/Lucene.java b/server/src/main/java/org/opensearch/common/lucene/Lucene.java index 2692a8fa2b914..54e86d3d347b2 100644 --- a/server/src/main/java/org/opensearch/common/lucene/Lucene.java +++ b/server/src/main/java/org/opensearch/common/lucene/Lucene.java @@ -67,6 +67,7 @@ import org.apache.lucene.index.SortedDocValues; import org.apache.lucene.index.SortedNumericDocValues; import org.apache.lucene.index.SortedSetDocValues; +import org.apache.lucene.index.StandardDirectoryReader; import org.apache.lucene.index.StoredFieldVisitor; import org.apache.lucene.index.Terms; import org.apache.lucene.index.VectorValues; @@ -162,6 +163,23 @@ public static SegmentInfos readSegmentInfos(Directory directory) throws IOExcept return SegmentInfos.readLatestCommit(directory); } + /** + * A variant of {@link #readSegmentInfos(Directory)} that supports reading indices written by + * older major versions of Lucene. The underlying implementation is a workaround since the + * "expert" readLatestCommit API is currently package-private in Lucene. First, all commits in + * the given {@link Directory} are listed - this result includes older Lucene commits. Then, + * the latest index commit is opened via {@link DirectoryReader} by including a minimum supported + * Lucene major version - this is based on the minimum index compatiblity version of OpenSearch. + */ + public static SegmentInfos readAnySegmentInfos(Directory directory) throws IOException { + // This list is sorted from oldest to latest + List indexCommits = DirectoryReader.listCommits(directory); + IndexCommit latestCommit = indexCommits.get(indexCommits.size() - 1); + int minSupportedLuceneMajor = org.opensearch.Version.CURRENT.minimumIndexCompatibilityVersion().luceneVersion.major; + StandardDirectoryReader reader = (StandardDirectoryReader) DirectoryReader.open(latestCommit, minSupportedLuceneMajor, null); + return reader.getSegmentInfos(); + } + /** * Returns an iterable that allows to iterate over all files in this segments info */ diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java index 31dd621f678ad..f5ab0a5a8de27 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -37,6 +37,13 @@ public class FeatureFlags { */ public static final String SEARCHABLE_SNAPSHOT = "opensearch.experimental.feature.searchable_snapshot.enabled"; + /** + * Gates the ability for Searchable Snapshots to read snapshots that are older than the + * guaranteed backward compatibilty for OpenSearch (one prior major version) on a best effort basis. + */ + public static final String SEARCHABLE_SNAPSHOT_EXTENDED_BWC = + "opensearch.experimental.feature.searchable_snapshot.extended_bwc.enabled"; + /** * Gates the functionality of extensions. * Once the feature is ready for production release, this feature flag can be removed. diff --git a/server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java b/server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java index ceea9eaac994c..c1c370d3a53dd 100644 --- a/server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java +++ b/server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java @@ -93,9 +93,24 @@ public class ReadOnlyEngine extends Engine { private final CompletionStatsCache completionStatsCache; private final boolean requireCompleteHistory; private final TranslogManager translogManager; + private final boolean allowOldIndices; 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 readerWrapperFunction, + boolean requireCompleteHistory + ) { + this(config, seqNoStats, translogStats, obtainLock, readerWrapperFunction, requireCompleteHistory, false); + } + /** * 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 @@ -115,10 +130,12 @@ public ReadOnlyEngine( TranslogStats translogStats, boolean obtainLock, Function readerWrapperFunction, - boolean requireCompleteHistory + boolean requireCompleteHistory, + boolean allowOldIndices ) { super(config); this.requireCompleteHistory = requireCompleteHistory; + this.allowOldIndices = allowOldIndices; try { Store store = config.getStore(); store.incRef(); @@ -130,7 +147,11 @@ public ReadOnlyEngine( // we obtain the IW lock even though we never modify the index. // yet this makes sure nobody else does. including some testing tools that try to be messy indexWriterLock = obtainLock ? directory.obtainLock(IndexWriter.WRITE_LOCK_NAME) : null; - this.lastCommittedSegmentInfos = Lucene.readSegmentInfos(directory); + if (this.allowOldIndices) { + this.lastCommittedSegmentInfos = Lucene.readAnySegmentInfos(directory); + } else { + this.lastCommittedSegmentInfos = Lucene.readSegmentInfos(directory); + } if (seqNoStats == null) { seqNoStats = buildSeqNoStats(config, lastCommittedSegmentInfos); ensureMaxSeqNoEqualsToGlobalCheckpoint(seqNoStats); @@ -223,7 +244,14 @@ protected final OpenSearchDirectoryReader wrapReader( protected DirectoryReader open(IndexCommit commit) throws IOException { assert Transports.assertNotTransportThread("opening index commit of a read-only engine"); - return new SoftDeletesDirectoryReaderWrapper(DirectoryReader.open(commit), Lucene.SOFT_DELETES_FIELD); + DirectoryReader reader; + if (this.allowOldIndices) { + int minSupportedLuceneMajor = org.opensearch.Version.CURRENT.minimumIndexCompatibilityVersion().luceneVersion.major; + reader = DirectoryReader.open(commit, minSupportedLuceneMajor, null); + } else { + reader = DirectoryReader.open(commit); + } + return new SoftDeletesDirectoryReaderWrapper(reader, Lucene.SOFT_DELETES_FIELD); } @Override diff --git a/server/src/main/java/org/opensearch/index/shard/IndexShard.java b/server/src/main/java/org/opensearch/index/shard/IndexShard.java index a108d1696ee93..c8e18c11160c6 100644 --- a/server/src/main/java/org/opensearch/index/shard/IndexShard.java +++ b/server/src/main/java/org/opensearch/index/shard/IndexShard.java @@ -85,6 +85,7 @@ import org.opensearch.common.unit.ByteSizeValue; import org.opensearch.common.unit.TimeValue; import org.opensearch.common.util.BigArrays; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.concurrent.AbstractRunnable; import org.opensearch.common.util.concurrent.AsyncIOProcessor; import org.opensearch.common.util.concurrent.RunOnce; @@ -1984,7 +1985,7 @@ public void openEngineAndRecoverFromTranslog() throws IOException { }; // Do not load the global checkpoint if this is a remote snapshot index - if (IndexModule.Type.REMOTE_SNAPSHOT.match(indexSettings) == false) { + if (isRemoteSnapshotIndex() == false) { loadGlobalCheckpointToReplicationTracker(); } @@ -2039,7 +2040,13 @@ private void innerOpenEngineAndTranslog(LongSupplier globalCheckpointSupplier) t } private boolean assertSequenceNumbersInCommit() throws IOException { - final Map userData = SegmentInfos.readLatestCommit(store.directory()).getUserData(); + final Map userData; + if (isRemoteSnapshotIndex() && FeatureFlags.isEnabled(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC)) { + // Inefficient method to support reading old Lucene indexes + userData = Lucene.readAnySegmentInfos(store.directory()).getUserData(); + } else { + userData = Lucene.readSegmentInfos(store.directory()).getUserData(); + } assert userData.containsKey(SequenceNumbers.LOCAL_CHECKPOINT_KEY) : "commit point doesn't contains a local checkpoint"; assert userData.containsKey(MAX_SEQ_NO) : "commit point doesn't contains a maximum sequence number"; assert userData.containsKey(Engine.HISTORY_UUID_KEY) : "commit point doesn't contains a history uuid"; @@ -2054,6 +2061,10 @@ private boolean assertSequenceNumbersInCommit() throws IOException { return true; } + private boolean isRemoteSnapshotIndex() { + return IndexModule.Type.REMOTE_SNAPSHOT.match(indexSettings); + } + private void onNewEngine(Engine newEngine) { assert Thread.holdsLock(engineMutex); refreshListeners.setCurrentRefreshLocationSupplier(newEngine::getTranslogLastWriteLocation); diff --git a/server/src/main/java/org/opensearch/index/store/Store.java b/server/src/main/java/org/opensearch/index/store/Store.java index 3354f7e8dbacb..8584cfba3de79 100644 --- a/server/src/main/java/org/opensearch/index/store/Store.java +++ b/server/src/main/java/org/opensearch/index/store/Store.java @@ -79,6 +79,7 @@ import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.unit.TimeValue; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.concurrent.AbstractRefCounted; import org.opensearch.common.util.concurrent.RefCounted; import org.opensearch.common.util.iterable.Iterables; @@ -86,6 +87,7 @@ import org.opensearch.env.NodeEnvironment; import org.opensearch.env.ShardLock; import org.opensearch.env.ShardLockObtainFailedException; +import org.opensearch.index.IndexModule; import org.opensearch.index.IndexSettings; import org.opensearch.index.engine.CombinedDeletionPolicy; import org.opensearch.index.engine.Engine; @@ -221,7 +223,12 @@ public Directory directory() { public SegmentInfos readLastCommittedSegmentsInfo() throws IOException { failIfCorrupted(); try { - return readSegmentsInfo(null, directory()); + if (IndexModule.Type.REMOTE_SNAPSHOT.match(indexSettings) + && FeatureFlags.isEnabled(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC)) { + return readAnySegmentsInfo(directory()); + } else { + return readSegmentsInfo(null, directory()); + } } catch (CorruptIndexException | IndexFormatTooOldException | IndexFormatTooNewException ex) { markStoreCorrupted(ex); throw ex; @@ -229,7 +236,9 @@ public SegmentInfos readLastCommittedSegmentsInfo() throws IOException { } /** - * Returns the segments info for the given commit or for the latest commit if the given commit is null + * Returns the segments info for the given commit or for the latest commit if the given commit is null. + * This method will throw an exception if the index is older than the standard backwards compatibility + * policy ( current major - 1). See also {@link #readAnySegmentsInfo(Directory)}. * * @throws IOException if the index is corrupted or the segments file is not present */ @@ -245,7 +254,26 @@ private static SegmentInfos readSegmentsInfo(IndexCommit commit, Directory direc } catch (Exception ex) { throw new CorruptIndexException("Hit unexpected exception while reading segment infos", "commit(" + commit + ")", ex); } + } + /** + * Returns the segments info for the latest commit in the given directory. Unlike + * {@link #readSegmentsInfo(IndexCommit, Directory)}, this method supports reading + * older Lucene indices on a best-effort basis. + * + * @throws IOException if the index is corrupted or the segments file is not present + */ + private static SegmentInfos readAnySegmentsInfo(Directory directory) throws IOException { + try { + return Lucene.readAnySegmentInfos(directory); + } 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", "", eof); + } catch (IOException exception) { + throw exception; // IOExceptions like too many open files are not necessarily a corruption - just bubble it up + } catch (Exception ex) { + throw new CorruptIndexException("Hit unexpected exception while reading segment infos", "", ex); + } } final void ensureOpen() { diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index afc2af302ed02..4b922736ed703 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesService.java +++ b/server/src/main/java/org/opensearch/indices/IndicesService.java @@ -79,6 +79,7 @@ import org.opensearch.common.unit.ByteSizeValue; import org.opensearch.common.unit.TimeValue; import org.opensearch.common.util.BigArrays; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.concurrent.AbstractRefCounted; import org.opensearch.common.util.concurrent.AbstractRunnable; import org.opensearch.common.util.concurrent.OpenSearchExecutors; @@ -888,7 +889,26 @@ private EngineFactory getEngineFactory(final IndexSettings idxSettings) { return new NRTReplicationEngineFactory(); } if (IndexModule.Type.REMOTE_SNAPSHOT.match(idxSettings)) { - return config -> new ReadOnlyEngine(config, new SeqNoStats(0, 0, 0), new TranslogStats(), true, Function.identity(), false); + if (FeatureFlags.isEnabled(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC)) { + return config -> new ReadOnlyEngine( + config, + new SeqNoStats(0, 0, 0), + new TranslogStats(), + true, + Function.identity(), + false, + true + ); + } else { + return config -> new ReadOnlyEngine( + config, + new SeqNoStats(0, 0, 0), + new TranslogStats(), + true, + Function.identity(), + false + ); + } } return new InternalEngineFactory(); } else if (engineFactories.size() == 1) { diff --git a/server/src/main/java/org/opensearch/repositories/blobstore/FileRestoreContext.java b/server/src/main/java/org/opensearch/repositories/blobstore/FileRestoreContext.java index d6cffcfbb8db8..8217e73c01a3c 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/FileRestoreContext.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/FileRestoreContext.java @@ -43,6 +43,7 @@ import org.opensearch.index.snapshots.blobstore.SnapshotFiles; import org.opensearch.index.store.Store; import org.opensearch.index.store.StoreFileMetadata; +import org.opensearch.index.store.remote.directory.RemoteSnapshotDirectory; import org.opensearch.indices.recovery.RecoveryState; import org.opensearch.snapshots.SnapshotId; @@ -198,6 +199,10 @@ public void restore(SnapshotFiles snapshotFiles, Store store, ActionListener) () -> System.setProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC, "true") + ); + try { + Version expected = LegacyESVersion.fromId(6000099); + // affects version 2+ + Version actual = Version.fromId(2000099); + assertEquals(expected.major, actual.minimumIndexCompatibilityVersion().major); + } finally { + AccessController.doPrivileged( + (PrivilegedAction) () -> System.clearProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC) + ); + } + } + /** test first version of opensearch compatibility that does not support legacy versions */ public void testOpenSearchPreLegacyRemoval() { Version opensearchVersion = Version.fromString("3.0.0"); From f4f1648942bf7b68296a7805a0431d3b973c5a88 Mon Sep 17 00:00:00 2001 From: Kartik Ganesh Date: Fri, 2 Dec 2022 09:41:07 -0800 Subject: [PATCH 02/10] Added some unit tests This change also includes a test index ZIP file for the unit tests. The change also introduces a bug fix in the readAnySegmentsInfo method to close the reader before returning the SegmentInfos object - this avoids dangling/open file handles. Signed-off-by: Kartik Ganesh --- .../org/opensearch/common/lucene/Lucene.java | 5 ++- .../opensearch/common/lucene/LuceneTests.java | 29 +++++++++++++++ .../index/engine/ReadOnlyEngineTests.java | 33 ++++++++++++++++++ .../resources/indices/bwc/testIndex-6.3.0.zip | Bin 0 -> 2467 bytes 4 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 server/src/test/resources/indices/bwc/testIndex-6.3.0.zip diff --git a/server/src/main/java/org/opensearch/common/lucene/Lucene.java b/server/src/main/java/org/opensearch/common/lucene/Lucene.java index 54e86d3d347b2..a98d5dcff56b6 100644 --- a/server/src/main/java/org/opensearch/common/lucene/Lucene.java +++ b/server/src/main/java/org/opensearch/common/lucene/Lucene.java @@ -177,7 +177,10 @@ public static SegmentInfos readAnySegmentInfos(Directory directory) throws IOExc IndexCommit latestCommit = indexCommits.get(indexCommits.size() - 1); int minSupportedLuceneMajor = org.opensearch.Version.CURRENT.minimumIndexCompatibilityVersion().luceneVersion.major; StandardDirectoryReader reader = (StandardDirectoryReader) DirectoryReader.open(latestCommit, minSupportedLuceneMajor, null); - return reader.getSegmentInfos(); + // cache the SegmentInfos object before closing the reader to prevent any open file handles + SegmentInfos si = reader.getSegmentInfos(); + reader.close(); + return si; } /** diff --git a/server/src/test/java/org/opensearch/common/lucene/LuceneTests.java b/server/src/test/java/org/opensearch/common/lucene/LuceneTests.java index 0edcd55cc35c3..5bcaddbc9e1d7 100644 --- a/server/src/test/java/org/opensearch/common/lucene/LuceneTests.java +++ b/server/src/test/java/org/opensearch/common/lucene/LuceneTests.java @@ -31,6 +31,7 @@ package org.opensearch.common.lucene; +import org.apache.lucene.index.IndexFormatTooOldException; import org.apache.lucene.tests.analysis.MockAnalyzer; import org.apache.lucene.analysis.core.KeywordAnalyzer; import org.apache.lucene.document.Document; @@ -71,10 +72,13 @@ import org.apache.lucene.store.Directory; import org.apache.lucene.store.MMapDirectory; import org.apache.lucene.tests.store.MockDirectoryWrapper; +import org.apache.lucene.tests.util.TestUtil; import org.apache.lucene.util.Bits; import org.apache.lucene.util.BytesRef; +import org.opensearch.common.SuppressForbidden; import org.opensearch.common.collect.Tuple; import org.opensearch.common.io.stream.NamedWriteableRegistry; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.internal.io.IOUtils; import org.opensearch.index.fielddata.IndexFieldData; import org.opensearch.index.fielddata.fieldcomparator.BytesRefFieldComparatorSource; @@ -87,6 +91,9 @@ import java.io.IOException; import java.io.StringReader; +import java.nio.file.Path; +import java.security.AccessController; +import java.security.PrivilegedAction; import java.util.ArrayList; import java.util.Collections; import java.util.HashSet; @@ -368,6 +375,28 @@ public void testNumDocs() throws IOException { dir.close(); } + @SuppressForbidden(reason = "tests readAnySegmentInfos with a feature flag enabled") + public void testReadAnySegmentInfos() throws IOException { + final String pathToTestIndex = "/indices/bwc/testIndex-6.3.0.zip"; + Path tmp = createTempDir(); + TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp); + MockDirectoryWrapper dir = newMockFSDirectory(tmp); + try (dir) { + // The standard API will throw an exception + expectThrows(IndexFormatTooOldException.class, () -> Lucene.readSegmentInfos(dir)); + // set the feature flag for extended backward compatibility + AccessController.doPrivileged( + (PrivilegedAction) () -> System.setProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC, "true") + ); + SegmentInfos si = Lucene.readAnySegmentInfos(dir); + assertEquals(1, Lucene.getNumDocs(si)); + } finally { + AccessController.doPrivileged( + (PrivilegedAction) () -> System.clearProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC) + ); + } + } + public void testCount() throws Exception { Directory dir = newDirectory(); RandomIndexWriter w = new RandomIndexWriter(random(), dir); diff --git a/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java b/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java index 5faa670ba4481..c609ae4ea36e8 100644 --- a/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java +++ b/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java @@ -33,10 +33,14 @@ import org.apache.lucene.index.DirectoryReader; import org.apache.lucene.index.IndexReader; +import org.apache.lucene.tests.store.BaseDirectoryWrapper; import org.apache.lucene.tests.util.LuceneTestCase; +import org.apache.lucene.tests.util.TestUtil; import org.opensearch.Version; +import org.opensearch.common.SuppressForbidden; import org.opensearch.common.bytes.BytesArray; import org.opensearch.common.lucene.index.OpenSearchDirectoryReader; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.internal.io.IOUtils; import org.opensearch.index.mapper.ParsedDocument; import org.opensearch.index.seqno.SeqNoStats; @@ -45,6 +49,9 @@ import org.opensearch.index.translog.TranslogStats; import java.io.IOException; +import java.nio.file.Path; +import java.security.AccessController; +import java.security.PrivilegedAction; import java.util.List; import java.util.concurrent.atomic.AtomicLong; import java.util.function.Function; @@ -228,6 +235,32 @@ public void testReadOnly() throws IOException { } } + @SuppressForbidden(reason = "tests creation of a ReadOnlyEngine with a feature flag enabled") + public void testReadOldIndices() 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); + final String pathToTestIndex = "/indices/bwc/testIndex-6.3.0.zip"; + Path tmp = createTempDir(); + TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp); + try (BaseDirectoryWrapper dir = newFSDirectory(tmp); Store store = createStore(dir)) { + // set the feature flag for extended backward compatibility + AccessController.doPrivileged( + (PrivilegedAction) () -> System.setProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC, "true") + ); + EngineConfig config = config(defaultSettings, store, createTempDir(), newMergePolicy(), null, null, globalCheckpoint::get); + try ( + ReadOnlyEngine readOnlyEngine = new ReadOnlyEngine(config, null, new TranslogStats(), true, Function.identity(), true, true) + ) { + assertVisibleCount(readOnlyEngine, 1, false); + } + } finally { + AccessController.doPrivileged( + (PrivilegedAction) () -> System.clearProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC) + ); + } + } + /** * Test that {@link ReadOnlyEngine#verifyEngineBeforeIndexClosing()} never fails * whatever the value of the global checkpoint to check is. diff --git a/server/src/test/resources/indices/bwc/testIndex-6.3.0.zip b/server/src/test/resources/indices/bwc/testIndex-6.3.0.zip new file mode 100644 index 0000000000000000000000000000000000000000..db86a76153b25b599dd5ba0a37042b1f44592e27 GIT binary patch literal 2467 zcmZ{mc|26>AIHbomnI^z#MqY@F&M?z`MH)t80%PO#+oc+@5)Y5GE9sy&DhsML=oB7 z<{}|=DZ5DBOehL>s^5LJyzc$uJkN8U^LoF|>-%}W=X^d^=8Q~y004ju@Y*%rPVGjQ z=rH~61_J=VLZ97ELQr0qhIXH%p>8XSxw{=ZOd#e4Y~(z~sGmP@%m$ z^=WHtzpdu!?QAdTJ6eA?zF+uN>7p!DFHRIcY=~1Hq$a10#9LkMl%p~4^jd9vunJ>P zjE=&{b8BS7ZAXd5%1Wg`1qbG%T>1@_0EJkPLc2&wB~zCdB-x3V=?NMXeo1gVO0mCi z?RSSw8&#;d2*{)2w@x2o!SI4<;!tX$n!e@y6lbG6$0AP!I9Jma!!c?Zb+Xfs?GIwC z9*V|U0c1h-Ix7RLeyX@`DkEu|2>{qV{6odqKkUsdrcI8K1usUlu^a(sCea4!eXAuH zHXzSv72K9X#Zr2vQwc)Gj;FTkG-nz+x5o+;#zYgwQ=f|{sHY-}?%p#QDov{@s4}%X ze5wQ&q7PR~C2QK+T^YBp8XFa?J@f)rRd2)Ex{$YX@YlhfuD6e5aD|72w8zo^CBg14 z7UkG1c|7K05Ij7r9kY>p`}#rT=FB}w;ab;S5iXuJnbc?}sD<%0VQh=1XN*HO$DMbe zFblo-@KF3#WDJQ?TUGPyUVBr`zMkPir(tS7(&vJ2fL8tfP@ z+|qxf#TEA;5P6yW7%Bl@^Bz??+^x4b7H8IU`{O`>&1&}YCutE=dyEAmPtd~5o0Da7 zNY|7i1YxWqHLW)zzY+De)^h0iTHFYMn6YHdEQ?0js!@aO`Veg5)I|Lpan=g>Q=Blb zOSVH=6j;ksq$EVV`|7X}d!B$Q6=h_M4y^yc5ukITZnYT`uzlw8qb5(W<_VWi+>|md zrCyyG4fp-r@+UnEY%$4APPF;*RhIde3ykpv60EGi&YKSG;Tj3WCU-%KxPWtK=Mn*} zO!nnAztz9}tCn#UTO`zvR$(t=zc_u1p|Oi6D4|t$JH9s)%spOx2%s>#E~UYx0wf1@ zz5;?s;tVhpvy1y_URpK4XEw}p-yGIS32Q)cUfICyM~sXZ7p<(AAYOp_7XsU&{2VQY zkvV6%Kq>lK);!aPQg$yUL&V3WlbrRU4YG~SJs!u-M(jtrMM#bDuChmjCmXo z3UJU6QEW5(F|@N%%W1EX@%SP~6&~T6WkX`ux;f-K}$(i6Czo z5!E^OSg?GbsN|`f-UKk`Dmy&LDc{@VWFz%HXvIF)gRb+pljfz^f0gRliSG7B5J- zBCq;pFN1IV^tG_`!RB7H_Dgu6CFzN4I!>^1Cu)grJ=Ehtc9kMrk0 zW0}zTGC{fz1kf+$Z(e}){b~j4->l#QH;G8jCnDzaa&t!h#Rd|4rs4^LmjwlJr`#DJ zWuHAkFMH6;2kyyiZ*TR~8@1Jsx8grbY-f&7j}=k1MJpVvgS{?^mKTzf$@;B#DtUB` zGzDb3G3Y8*zDY~fy4gnz54dEy{c5~#c@L1?Zsj`QNuBl{9{hn`62M8MEfJBG$B7u^ zgdIn7IR{_zwGL76cRJ%6%pzc=Mb$QBgd0N7WcJNx^-KeihES-vxg;+muOt@B>(vy= z*tU4r)*fs}VxQxK4jBR;jf3w?wacmZDnZp{OQ)3DqEuY+uW4jynG!!bJ`bFtfB>#)M8``QU$V`6adb(G7!6-iRhFLRz_7Wvm6ux zO6GKh0Qp@qnb{N)ftNzU5lOC@lsrV24>&vDfya%AAQ5teC=O0pVwAiiZdrNxPA*vl z=Z+Hp0cRUL0YS>9h`Kou^Cb8&F2`mC*T@3v6kn1L>Ghg}*E*ViARFiClI3R0ZoP0s zY!>OUD9VGkIfh*FuqUH(#5}DaTrj!gubvn6WKlnBo1h?>TfLRZ8&Nw~SydK$O3gIRj9X@xL?3KV(V#m({mK@+ Date: Mon, 5 Dec 2022 18:49:28 -0800 Subject: [PATCH 03/10] Incorporating PR feedback Signed-off-by: Kartik Ganesh --- CHANGELOG.md | 1 + .../src/main/java/org/opensearch/Version.java | 6 -- .../cluster/block/ClusterBlocks.java | 3 +- .../cluster/metadata/IndexMetadata.java | 2 - .../cluster/routing/RoutingPool.java | 2 +- .../org/opensearch/common/lucene/Lucene.java | 15 ++- .../common/settings/IndexScopedSettings.java | 4 +- .../opensearch/common/util/FeatureFlags.java | 6 +- .../org/opensearch/index/IndexSettings.java | 32 ++++++ .../index/engine/ReadOnlyEngine.java | 30 ++++-- .../opensearch/index/shard/IndexShard.java | 21 ++-- .../org/opensearch/index/store/Store.java | 14 ++- .../opensearch/indices/IndicesService.java | 32 ++---- .../recovery/PeerRecoveryTargetService.java | 3 +- .../indices/recovery/RecoveryTarget.java | 3 +- .../opensearch/snapshots/RestoreService.java | 35 ++++-- .../opensearch/snapshots/SnapshotUtils.java | 2 +- .../java/org/opensearch/VersionTests.java | 21 ---- .../opensearch/common/lucene/LuceneTests.java | 101 ++++++++++++++---- .../opensearch/index/IndexSettingsTests.java | 44 ++++++++ .../index/engine/ReadOnlyEngineTests.java | 37 +++---- .../opensearch/index/store/StoreTests.java | 59 ++++++++++ .../resources/indices/bwc/testIndex-6.3.0.md | 57 ++++++++++ 23 files changed, 383 insertions(+), 147 deletions(-) create mode 100644 server/src/test/resources/indices/bwc/testIndex-6.3.0.md diff --git a/CHANGELOG.md b/CHANGELOG.md index ab3266e44ace2..3e17b4725c0dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Added experimental support for extensions ([#5347](https://github.com/opensearch-project/OpenSearch/pull/5347)), ([#5518](https://github.com/opensearch-project/OpenSearch/pull/5518)), ([#5597](https://github.com/opensearch-project/OpenSearch/pull/5597)), ([#5615](https://github.com/opensearch-project/OpenSearch/pull/5615))) - Add CI bundle pattern to distribution download ([#5348](https://github.com/opensearch-project/OpenSearch/pull/5348)) - Added @gbbafna as an OpenSearch maintainer ([#5668](https://github.com/opensearch-project/OpenSearch/pull/5668)) +- Experimental support for extended backward compatiblity in searchable snapshots ([#5429](https://github.com/opensearch-project/OpenSearch/pull/5429)) ### Dependencies - Bump bcpg-fips from 1.0.5.1 to 1.0.7.1 ([#5148](https://github.com/opensearch-project/OpenSearch/pull/5148)) diff --git a/server/src/main/java/org/opensearch/Version.java b/server/src/main/java/org/opensearch/Version.java index 9162f84fbea8f..6c105a6233126 100644 --- a/server/src/main/java/org/opensearch/Version.java +++ b/server/src/main/java/org/opensearch/Version.java @@ -38,7 +38,6 @@ import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.io.stream.StreamOutput; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.xcontent.ToXContentFragment; import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.monitor.jvm.JvmInfo; @@ -52,8 +51,6 @@ import java.util.Locale; import java.util.Objects; -import static org.opensearch.common.util.FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC; - /** * OpenSearch Version Class * @@ -418,9 +415,6 @@ private Version computeMinIndexCompatVersion() { } else if (major == 7 || major == 1) { return LegacyESVersion.fromId(6000026); } else if (major == 2) { - if (FeatureFlags.isEnabled(SEARCHABLE_SNAPSHOT_EXTENDED_BWC)) { - return LegacyESVersion.fromId(6030099); - } return LegacyESVersion.V_7_0_0; } else { bwcMajor = major - 1; diff --git a/server/src/main/java/org/opensearch/cluster/block/ClusterBlocks.java b/server/src/main/java/org/opensearch/cluster/block/ClusterBlocks.java index 13d8909b41ff5..1a49e18713901 100644 --- a/server/src/main/java/org/opensearch/cluster/block/ClusterBlocks.java +++ b/server/src/main/java/org/opensearch/cluster/block/ClusterBlocks.java @@ -394,8 +394,7 @@ public Builder addBlocks(IndexMetadata indexMetadata) { if (IndexMetadata.INDEX_BLOCKS_READ_ONLY_ALLOW_DELETE_SETTING.get(indexMetadata.getSettings())) { addIndexBlock(indexName, IndexMetadata.INDEX_READ_ONLY_ALLOW_DELETE_BLOCK); } - if (IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey() - .equals(indexMetadata.getSettings().get(IndexModule.INDEX_STORE_TYPE_SETTING.getKey()))) { + if (IndexModule.Type.REMOTE_SNAPSHOT.match(indexMetadata.getSettings().get(IndexModule.INDEX_STORE_TYPE_SETTING.getKey()))) { addIndexBlock(indexName, IndexMetadata.REMOTE_READ_ONLY_ALLOW_DELETE); } return this; diff --git a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java index 71f2666c2371c..4a3cad9e5a7b2 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java @@ -1830,8 +1830,6 @@ public static IndexMetadata fromXContent(XContentParser parser) throws IOExcepti Version legacyVersion = LegacyESVersion.fromId(6050099); if (Assertions.ENABLED && Version.indexCreated(builder.settings).onOrAfter(legacyVersion)) { assert mappingVersion : "mapping version should be present for indices"; - } - if (Assertions.ENABLED && Version.indexCreated(builder.settings).onOrAfter(legacyVersion)) { assert settingsVersion : "settings version should be present for indices"; } if (Assertions.ENABLED && Version.indexCreated(builder.settings).onOrAfter(LegacyESVersion.V_7_2_0)) { diff --git a/server/src/main/java/org/opensearch/cluster/routing/RoutingPool.java b/server/src/main/java/org/opensearch/cluster/routing/RoutingPool.java index 1a3c366694221..a4ff237460e28 100644 --- a/server/src/main/java/org/opensearch/cluster/routing/RoutingPool.java +++ b/server/src/main/java/org/opensearch/cluster/routing/RoutingPool.java @@ -61,7 +61,7 @@ public static RoutingPool getShardPool(ShardRouting shard, RoutingAllocation all */ public static RoutingPool getIndexPool(IndexMetadata indexMetadata) { Settings indexSettings = indexMetadata.getSettings(); - if (IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey().equals(indexSettings.get(IndexModule.INDEX_STORE_TYPE_SETTING.getKey()))) { + if (IndexModule.Type.REMOTE_SNAPSHOT.match(indexSettings.get(IndexModule.INDEX_STORE_TYPE_SETTING.getKey()))) { return REMOTE_CAPABLE; } return LOCAL_ONLY; diff --git a/server/src/main/java/org/opensearch/common/lucene/Lucene.java b/server/src/main/java/org/opensearch/common/lucene/Lucene.java index a98d5dcff56b6..870293e425f8a 100644 --- a/server/src/main/java/org/opensearch/common/lucene/Lucene.java +++ b/server/src/main/java/org/opensearch/common/lucene/Lucene.java @@ -169,18 +169,17 @@ public static SegmentInfos readSegmentInfos(Directory directory) throws IOExcept * "expert" readLatestCommit API is currently package-private in Lucene. First, all commits in * the given {@link Directory} are listed - this result includes older Lucene commits. Then, * the latest index commit is opened via {@link DirectoryReader} by including a minimum supported - * Lucene major version - this is based on the minimum index compatiblity version of OpenSearch. + * Lucene major version based on the minimum compatibility of the given {@link org.opensearch.Version}. */ - public static SegmentInfos readAnySegmentInfos(Directory directory) throws IOException { + public static SegmentInfos readSegmentInfosExtendedCompatibility(Directory directory, org.opensearch.Version minimumVersion) + throws IOException { // This list is sorted from oldest to latest List indexCommits = DirectoryReader.listCommits(directory); IndexCommit latestCommit = indexCommits.get(indexCommits.size() - 1); - int minSupportedLuceneMajor = org.opensearch.Version.CURRENT.minimumIndexCompatibilityVersion().luceneVersion.major; - StandardDirectoryReader reader = (StandardDirectoryReader) DirectoryReader.open(latestCommit, minSupportedLuceneMajor, null); - // cache the SegmentInfos object before closing the reader to prevent any open file handles - SegmentInfos si = reader.getSegmentInfos(); - reader.close(); - return si; + final int minSupportedLuceneMajor = minimumVersion.minimumIndexCompatibilityVersion().luceneVersion.major; + try (StandardDirectoryReader reader = (StandardDirectoryReader) DirectoryReader.open(latestCommit, minSupportedLuceneMajor, null)) { + return reader.getSegmentInfos(); + } } /** diff --git a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java index 079fc38415328..900271431e6c2 100644 --- a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java @@ -234,7 +234,9 @@ public final class IndexScopedSettings extends AbstractScopedSettings { IndexSettings.SEARCHABLE_SNAPSHOT_INDEX_ID, IndexSettings.SEARCHABLE_SNAPSHOT_ID_NAME, IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID - ) + ), + FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY, + List.of(IndexSettings.SEARCHABLE_SNAPSHOT_MINIMUM_VERSION) ); public static final IndexScopedSettings DEFAULT_SCOPED_SETTINGS = new IndexScopedSettings(Settings.EMPTY, BUILT_IN_INDEX_SETTINGS); diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java index f5ab0a5a8de27..9f1a9a43ff54e 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -39,10 +39,10 @@ public class FeatureFlags { /** * Gates the ability for Searchable Snapshots to read snapshots that are older than the - * guaranteed backward compatibilty for OpenSearch (one prior major version) on a best effort basis. + * guaranteed backward compatibility for OpenSearch (one prior major version) on a best effort basis. */ - public static final String SEARCHABLE_SNAPSHOT_EXTENDED_BWC = - "opensearch.experimental.feature.searchable_snapshot.extended_bwc.enabled"; + public static final String SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY = + "opensearch.experimental.feature.searchable_snapshot.extended_compatibility.enabled"; /** * Gates the functionality of extensions. diff --git a/server/src/main/java/org/opensearch/index/IndexSettings.java b/server/src/main/java/org/opensearch/index/IndexSettings.java index 72602077150b4..bda5ddf9741a5 100644 --- a/server/src/main/java/org/opensearch/index/IndexSettings.java +++ b/server/src/main/java/org/opensearch/index/IndexSettings.java @@ -575,6 +575,13 @@ public final class IndexSettings { Property.InternalIndex ); + public static final Setting SEARCHABLE_SNAPSHOT_MINIMUM_VERSION = Setting.intSetting( + "index.searchable_snapshot.minversion", + 0, + Property.IndexScope, + Property.InternalIndex + ); + private final Index index; private final Version version; private final Logger logger; @@ -585,6 +592,8 @@ public final class IndexSettings { private final boolean isRemoteStoreEnabled; private final String remoteStoreRepository; private final boolean isRemoteTranslogStoreEnabled; + private final boolean isRemoteSnapshot; + private Version extendedCompatibilitySnapshotVersion; // volatile fields are updated via #updateIndexMetadata(IndexMetadata) under lock private volatile Settings settings; private volatile IndexMetadata indexMetadata; @@ -747,6 +756,13 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti isRemoteStoreEnabled = settings.getAsBoolean(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false); remoteStoreRepository = settings.get(IndexMetadata.SETTING_REMOTE_STORE_REPOSITORY); 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(), Version.V_EMPTY_ID); + if (minVersion > Version.V_EMPTY_ID) { + extendedCompatibilitySnapshotVersion = LegacyESVersion.fromId(minVersion); + } + } this.searchThrottled = INDEX_SEARCH_THROTTLED.get(settings); this.queryStringLenient = QUERY_STRING_LENIENT_SETTING.get(settings); this.queryStringAnalyzeWildcard = QUERY_STRING_ANALYZE_WILDCARD.get(nodeSettings); @@ -1012,6 +1028,22 @@ public boolean isRemoteTranslogStoreEnabled() { return isRemoteTranslogStoreEnabled; } + /** + * Returns true if this is remote/searchable snapshot + */ + public boolean isRemoteSnapshot() { + return isRemoteSnapshot; + } + + /** + * If this is a remote snapshot and the extended compatibility + * feature flag is enabled, this returns the minimum {@link Version} + * supported. In all other cases, the return value is null. + */ + public Version getExtendedCompatibilitySnapshotVersion() { + return extendedCompatibilitySnapshotVersion; + } + /** * Returns the node settings. The settings returned from {@link #getSettings()} are a merged version of the * index settings and the node settings where node settings are overwritten by index settings. diff --git a/server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java b/server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java index c1c370d3a53dd..6d53787c989a3 100644 --- a/server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java +++ b/server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java @@ -93,7 +93,7 @@ public class ReadOnlyEngine extends Engine { private final CompletionStatsCache completionStatsCache; private final boolean requireCompleteHistory; private final TranslogManager translogManager; - private final boolean allowOldIndices; + private final Version minimumSupportedVersion; protected volatile TranslogStats translogStats; @@ -108,7 +108,15 @@ public ReadOnlyEngine( Function readerWrapperFunction, boolean requireCompleteHistory ) { - this(config, seqNoStats, translogStats, obtainLock, readerWrapperFunction, requireCompleteHistory, false); + this( + config, + seqNoStats, + translogStats, + obtainLock, + readerWrapperFunction, + requireCompleteHistory, + Version.CURRENT.minimumIndexCompatibilityVersion() + ); } /** @@ -123,6 +131,7 @@ 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 < MSN) + * @param minimumSupportedVersion for extended backward compatibility use-cases, the minimum {@link Version} this engine is allowed to read */ public ReadOnlyEngine( EngineConfig config, @@ -131,11 +140,11 @@ public ReadOnlyEngine( boolean obtainLock, Function readerWrapperFunction, boolean requireCompleteHistory, - boolean allowOldIndices + Version minimumSupportedVersion ) { super(config); this.requireCompleteHistory = requireCompleteHistory; - this.allowOldIndices = allowOldIndices; + this.minimumSupportedVersion = minimumSupportedVersion; try { Store store = config.getStore(); store.incRef(); @@ -147,8 +156,8 @@ public ReadOnlyEngine( // we obtain the IW lock even though we never modify the index. // yet this makes sure nobody else does. including some testing tools that try to be messy indexWriterLock = obtainLock ? directory.obtainLock(IndexWriter.WRITE_LOCK_NAME) : null; - if (this.allowOldIndices) { - this.lastCommittedSegmentInfos = Lucene.readAnySegmentInfos(directory); + if (isExtendedCompatibility()) { + this.lastCommittedSegmentInfos = Lucene.readSegmentInfosExtendedCompatibility(directory, this.minimumSupportedVersion); } else { this.lastCommittedSegmentInfos = Lucene.readSegmentInfos(directory); } @@ -245,15 +254,18 @@ protected final OpenSearchDirectoryReader wrapReader( protected DirectoryReader open(IndexCommit commit) throws IOException { assert Transports.assertNotTransportThread("opening index commit of a read-only engine"); DirectoryReader reader; - if (this.allowOldIndices) { - int minSupportedLuceneMajor = org.opensearch.Version.CURRENT.minimumIndexCompatibilityVersion().luceneVersion.major; - reader = DirectoryReader.open(commit, minSupportedLuceneMajor, null); + if (isExtendedCompatibility()) { + reader = DirectoryReader.open(commit, this.minimumSupportedVersion.luceneVersion.major, null); } else { reader = DirectoryReader.open(commit); } return new SoftDeletesDirectoryReaderWrapper(reader, Lucene.SOFT_DELETES_FIELD); } + private boolean isExtendedCompatibility() { + return this.minimumSupportedVersion.before(Version.CURRENT.minimumIndexCompatibilityVersion()); + } + @Override protected void closeNoLock(String reason, CountDownLatch closedLatch) { if (isClosed.compareAndSet(false, true)) { diff --git a/server/src/main/java/org/opensearch/index/shard/IndexShard.java b/server/src/main/java/org/opensearch/index/shard/IndexShard.java index c8e18c11160c6..a800f26a58c08 100644 --- a/server/src/main/java/org/opensearch/index/shard/IndexShard.java +++ b/server/src/main/java/org/opensearch/index/shard/IndexShard.java @@ -85,7 +85,6 @@ import org.opensearch.common.unit.ByteSizeValue; import org.opensearch.common.unit.TimeValue; import org.opensearch.common.util.BigArrays; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.concurrent.AbstractRunnable; import org.opensearch.common.util.concurrent.AsyncIOProcessor; import org.opensearch.common.util.concurrent.RunOnce; @@ -1985,7 +1984,7 @@ public void openEngineAndRecoverFromTranslog() throws IOException { }; // Do not load the global checkpoint if this is a remote snapshot index - if (isRemoteSnapshotIndex() == false) { + if (indexSettings.isRemoteSnapshot() == false) { loadGlobalCheckpointToReplicationTracker(); } @@ -2040,13 +2039,7 @@ private void innerOpenEngineAndTranslog(LongSupplier globalCheckpointSupplier) t } private boolean assertSequenceNumbersInCommit() throws IOException { - final Map userData; - if (isRemoteSnapshotIndex() && FeatureFlags.isEnabled(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC)) { - // Inefficient method to support reading old Lucene indexes - userData = Lucene.readAnySegmentInfos(store.directory()).getUserData(); - } else { - userData = Lucene.readSegmentInfos(store.directory()).getUserData(); - } + final Map userData = fetchUserData(); assert userData.containsKey(SequenceNumbers.LOCAL_CHECKPOINT_KEY) : "commit point doesn't contains a local checkpoint"; assert userData.containsKey(MAX_SEQ_NO) : "commit point doesn't contains a maximum sequence number"; assert userData.containsKey(Engine.HISTORY_UUID_KEY) : "commit point doesn't contains a history uuid"; @@ -2061,8 +2054,14 @@ private boolean assertSequenceNumbersInCommit() throws IOException { return true; } - private boolean isRemoteSnapshotIndex() { - return IndexModule.Type.REMOTE_SNAPSHOT.match(indexSettings); + private Map fetchUserData() throws IOException { + if (indexSettings.isRemoteSnapshot() && indexSettings.getExtendedCompatibilitySnapshotVersion() != null) { + // Inefficient method to support reading old Lucene indexes + return Lucene.readSegmentInfosExtendedCompatibility(store.directory(), indexSettings.getExtendedCompatibilitySnapshotVersion()) + .getUserData(); + } else { + return SegmentInfos.readLatestCommit(store.directory()).getUserData(); + } } private void onNewEngine(Engine newEngine) { diff --git a/server/src/main/java/org/opensearch/index/store/Store.java b/server/src/main/java/org/opensearch/index/store/Store.java index 8584cfba3de79..de12d9b75dc12 100644 --- a/server/src/main/java/org/opensearch/index/store/Store.java +++ b/server/src/main/java/org/opensearch/index/store/Store.java @@ -79,7 +79,6 @@ import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.unit.TimeValue; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.concurrent.AbstractRefCounted; import org.opensearch.common.util.concurrent.RefCounted; import org.opensearch.common.util.iterable.Iterables; @@ -87,7 +86,6 @@ import org.opensearch.env.NodeEnvironment; import org.opensearch.env.ShardLock; import org.opensearch.env.ShardLockObtainFailedException; -import org.opensearch.index.IndexModule; import org.opensearch.index.IndexSettings; import org.opensearch.index.engine.CombinedDeletionPolicy; import org.opensearch.index.engine.Engine; @@ -223,9 +221,8 @@ public Directory directory() { public SegmentInfos readLastCommittedSegmentsInfo() throws IOException { failIfCorrupted(); try { - if (IndexModule.Type.REMOTE_SNAPSHOT.match(indexSettings) - && FeatureFlags.isEnabled(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC)) { - return readAnySegmentsInfo(directory()); + if (indexSettings.isRemoteSnapshot() && indexSettings.getExtendedCompatibilitySnapshotVersion() != null) { + return readSegmentInfosExtendedCompatibility(directory(), indexSettings.getExtendedCompatibilitySnapshotVersion()); } else { return readSegmentsInfo(null, directory()); } @@ -238,7 +235,7 @@ public SegmentInfos readLastCommittedSegmentsInfo() throws IOException { /** * Returns the segments info for the given commit or for the latest commit if the given commit is null. * This method will throw an exception if the index is older than the standard backwards compatibility - * policy ( current major - 1). See also {@link #readAnySegmentsInfo(Directory)}. + * policy ( current major - 1). See also {@link #readSegmentInfosExtendedCompatibility(Directory, org.opensearch.Version)}. * * @throws IOException if the index is corrupted or the segments file is not present */ @@ -263,9 +260,10 @@ private static SegmentInfos readSegmentsInfo(IndexCommit commit, Directory direc * * @throws IOException if the index is corrupted or the segments file is not present */ - private static SegmentInfos readAnySegmentsInfo(Directory directory) throws IOException { + private static SegmentInfos readSegmentInfosExtendedCompatibility(Directory directory, org.opensearch.Version minimumVersion) + throws IOException { try { - return Lucene.readAnySegmentInfos(directory); + 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", "", eof); diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index 4b922736ed703..29065936f4e15 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesService.java +++ b/server/src/main/java/org/opensearch/indices/IndicesService.java @@ -79,7 +79,6 @@ import org.opensearch.common.unit.ByteSizeValue; import org.opensearch.common.unit.TimeValue; import org.opensearch.common.util.BigArrays; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.concurrent.AbstractRefCounted; import org.opensearch.common.util.concurrent.AbstractRunnable; import org.opensearch.common.util.concurrent.OpenSearchExecutors; @@ -888,27 +887,16 @@ private EngineFactory getEngineFactory(final IndexSettings idxSettings) { if (idxSettings.isSegRepEnabled()) { return new NRTReplicationEngineFactory(); } - if (IndexModule.Type.REMOTE_SNAPSHOT.match(idxSettings)) { - if (FeatureFlags.isEnabled(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC)) { - return config -> new ReadOnlyEngine( - config, - new SeqNoStats(0, 0, 0), - new TranslogStats(), - true, - Function.identity(), - false, - true - ); - } else { - return config -> new ReadOnlyEngine( - config, - new SeqNoStats(0, 0, 0), - new TranslogStats(), - true, - Function.identity(), - false - ); - } + if (idxSettings.isRemoteSnapshot()) { + 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/main/java/org/opensearch/indices/recovery/PeerRecoveryTargetService.java b/server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryTargetService.java index 556e4db3400e1..afee371adc66c 100644 --- a/server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryTargetService.java +++ b/server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryTargetService.java @@ -54,7 +54,6 @@ import org.opensearch.common.unit.TimeValue; import org.opensearch.common.util.CancellableThreads; import org.opensearch.common.util.concurrent.AbstractRunnable; -import org.opensearch.index.IndexModule; import org.opensearch.index.IndexNotFoundException; import org.opensearch.index.engine.RecoveryEngineException; import org.opensearch.index.mapper.MapperException; @@ -246,7 +245,7 @@ private void doRecovery(final long recoveryId, final StartRecoveryRequest preExi logger.trace("{} preparing shard for peer recovery", recoveryTarget.shardId()); indexShard.prepareForIndexRecovery(); final boolean hasRemoteTranslog = recoveryTarget.state().getPrimary() == false && indexShard.isRemoteTranslogEnabled(); - final boolean hasNoTranslog = IndexModule.Type.REMOTE_SNAPSHOT.match(indexShard.indexSettings()); + final boolean hasNoTranslog = indexShard.indexSettings().isRemoteSnapshot(); final boolean verifyTranslog = (hasRemoteTranslog || hasNoTranslog) == false; final long startingSeqNo = indexShard.recoverLocallyAndFetchStartSeqNo(!hasRemoteTranslog); assert startingSeqNo == UNASSIGNED_SEQ_NO || recoveryTarget.state().getStage() == RecoveryState.Stage.TRANSLOG diff --git a/server/src/main/java/org/opensearch/indices/recovery/RecoveryTarget.java b/server/src/main/java/org/opensearch/indices/recovery/RecoveryTarget.java index c661ad3a461fe..62aad1ce263ad 100644 --- a/server/src/main/java/org/opensearch/indices/recovery/RecoveryTarget.java +++ b/server/src/main/java/org/opensearch/indices/recovery/RecoveryTarget.java @@ -45,7 +45,6 @@ import org.opensearch.common.bytes.BytesReference; import org.opensearch.common.lucene.Lucene; import org.opensearch.common.util.CancellableThreads; -import org.opensearch.index.IndexModule; import org.opensearch.index.engine.Engine; import org.opensearch.index.mapper.MapperException; import org.opensearch.index.seqno.ReplicationTracker; @@ -407,7 +406,7 @@ public void cleanFiles( // their own commit points and therefore do not modify the commit user data // in their store. In these cases, reuse the primary's translog UUID. final boolean reuseTranslogUUID = indexShard.indexSettings().isSegRepEnabled() - || IndexModule.Type.REMOTE_SNAPSHOT.match(indexShard.indexSettings()); + || indexShard.indexSettings().isRemoteSnapshot(); if (reuseTranslogUUID) { final String translogUUID = store.getMetadata().getCommitUserData().get(TRANSLOG_UUID_KEY); Translog.createEmptyTranslog( diff --git a/server/src/main/java/org/opensearch/snapshots/RestoreService.java b/server/src/main/java/org/opensearch/snapshots/RestoreService.java index 9a4c3e7dc8ce8..b3c6d5f84af81 100644 --- a/server/src/main/java/org/opensearch/snapshots/RestoreService.java +++ b/server/src/main/java/org/opensearch/snapshots/RestoreService.java @@ -121,6 +121,7 @@ import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_VERSION_CREATED; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_VERSION_UPGRADED; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED; +import static org.opensearch.common.util.FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY; import static org.opensearch.common.util.set.Sets.newHashSet; import static org.opensearch.snapshots.SnapshotUtils.filterIndices; @@ -442,9 +443,7 @@ public ClusterState execute(ClusterState currentState) { // We have some indices to restore ImmutableOpenMap.Builder shardsBuilder = ImmutableOpenMap .builder(); - final Version minIndexCompatibilityVersion = currentState.getNodes() - .getMaxNodeVersion() - .minimumIndexCompatibilityVersion(); + for (Map.Entry indexEntry : indices.entrySet()) { String renamedIndexName = indexEntry.getKey(); String index = indexEntry.getValue(); @@ -456,7 +455,7 @@ public ClusterState execute(ClusterState currentState) { request.ignoreIndexSettings() ); final boolean isSearchableSnapshot = FeatureFlags.isEnabled(FeatureFlags.SEARCHABLE_SNAPSHOT) - && IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey().equals(request.storageType().toString()); + && IndexModule.Type.REMOTE_SNAPSHOT.match(request.storageType().toString()); if (isSearchableSnapshot) { snapshotIndexMetadata = addSnapshotToIndexSettings( snapshotIndexMetadata, @@ -471,6 +470,17 @@ public ClusterState execute(ClusterState currentState) { repositoryData.resolveIndexId(index), isSearchableSnapshot ); + final Version minIndexCompatibilityVersion; + if (isSearchableSnapshot && isSearchableSnapshotsExtendedCompatibilityEnabled()) { + final int minVersion = IndexSettings.SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.get( + snapshotIndexMetadata.getSettings() + ); + minIndexCompatibilityVersion = LegacyESVersion.fromId(minVersion).minimumIndexCompatibilityVersion(); + } else { + minIndexCompatibilityVersion = currentState.getNodes() + .getMaxNodeVersion() + .minimumIndexCompatibilityVersion(); + } try { snapshotIndexMetadata = metadataIndexUpgradeService.upgradeIndexMetadata( snapshotIndexMetadata, @@ -1244,14 +1254,23 @@ public void applyClusterState(ClusterChangedEvent event) { } private static IndexMetadata addSnapshotToIndexSettings(IndexMetadata metadata, Snapshot snapshot, IndexId indexId) { - final Settings newSettings = Settings.builder() + final Settings.Builder settingsBuilder = Settings.builder() .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) .put(IndexSettings.SEARCHABLE_SNAPSHOT_REPOSITORY.getKey(), snapshot.getRepository()) .put(IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID.getKey(), snapshot.getSnapshotId().getUUID()) .put(IndexSettings.SEARCHABLE_SNAPSHOT_ID_NAME.getKey(), snapshot.getSnapshotId().getName()) .put(IndexSettings.SEARCHABLE_SNAPSHOT_INDEX_ID.getKey(), indexId.getId()) - .put(metadata.getSettings()) - .build(); - return IndexMetadata.builder(metadata).settings(newSettings).build(); + .put(metadata.getSettings()); + // Check for extended backwards compatibility feature flag + if (isSearchableSnapshotsExtendedCompatibilityEnabled()) { + // The oldest snapshot version we can read is ES 6.0 + settingsBuilder.put(IndexSettings.SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.getKey(), 6000099); + } + return IndexMetadata.builder(metadata).settings(settingsBuilder.build()).build(); + } + + private static boolean isSearchableSnapshotsExtendedCompatibilityEnabled() { + return org.opensearch.Version.CURRENT.after(org.opensearch.Version.V_2_4_0) + && FeatureFlags.isEnabled(SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY); } } diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotUtils.java b/server/src/main/java/org/opensearch/snapshots/SnapshotUtils.java index 5c2efba008652..2be7cf9d4dbb3 100644 --- a/server/src/main/java/org/opensearch/snapshots/SnapshotUtils.java +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotUtils.java @@ -173,7 +173,7 @@ public static void validateSnapshotsBackingAnyIndex( for (ObjectCursor cursor : metadata.values()) { IndexMetadata indexMetadata = cursor.value; String storeType = indexMetadata.getSettings().get(IndexModule.INDEX_STORE_TYPE_SETTING.getKey()); - if (IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey().equals(storeType)) { + if (IndexModule.Type.REMOTE_SNAPSHOT.match(storeType)) { String snapshotId = indexMetadata.getSettings().get(IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID.getKey()); if (uuidToSnapshotId.get(snapshotId) != null) { snapshotsToBeNotDeleted.add(uuidToSnapshotId.get(snapshotId).getName()); diff --git a/server/src/test/java/org/opensearch/VersionTests.java b/server/src/test/java/org/opensearch/VersionTests.java index edb2095b8d825..c2566e83dd9b6 100644 --- a/server/src/test/java/org/opensearch/VersionTests.java +++ b/server/src/test/java/org/opensearch/VersionTests.java @@ -33,17 +33,13 @@ package org.opensearch; import org.opensearch.cluster.metadata.IndexMetadata; -import org.opensearch.common.SuppressForbidden; import org.opensearch.common.lucene.Lucene; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.test.VersionUtils; import org.hamcrest.Matchers; import java.lang.reflect.Modifier; -import java.security.AccessController; -import java.security.PrivilegedAction; import java.util.ArrayList; import java.util.Comparator; import java.util.HashMap; @@ -249,23 +245,6 @@ public void testOpenSearchMinIndexCompatVersion() { assertEquals(expected.revision, actual.revision); } - @SuppressForbidden(reason = "tests minimum index compatiblity with a feature flag enabled") - public void testMinimumIndexCompatWithFeatureFlag() { - AccessController.doPrivileged( - (PrivilegedAction) () -> System.setProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC, "true") - ); - try { - Version expected = LegacyESVersion.fromId(6000099); - // affects version 2+ - Version actual = Version.fromId(2000099); - assertEquals(expected.major, actual.minimumIndexCompatibilityVersion().major); - } finally { - AccessController.doPrivileged( - (PrivilegedAction) () -> System.clearProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC) - ); - } - } - /** test first version of opensearch compatibility that does not support legacy versions */ public void testOpenSearchPreLegacyRemoval() { Version opensearchVersion = Version.fromString("3.0.0"); diff --git a/server/src/test/java/org/opensearch/common/lucene/LuceneTests.java b/server/src/test/java/org/opensearch/common/lucene/LuceneTests.java index 5bcaddbc9e1d7..d4cf1fb1836e6 100644 --- 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; @@ -75,10 +78,10 @@ import org.apache.lucene.tests.util.TestUtil; import org.apache.lucene.util.Bits; import org.apache.lucene.util.BytesRef; -import org.opensearch.common.SuppressForbidden; +import org.opensearch.LegacyESVersion; +import org.opensearch.Version; import org.opensearch.common.collect.Tuple; import org.opensearch.common.io.stream.NamedWriteableRegistry; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.internal.io.IOUtils; import org.opensearch.index.fielddata.IndexFieldData; import org.opensearch.index.fielddata.fieldcomparator.BytesRefFieldComparatorSource; @@ -92,8 +95,6 @@ import java.io.IOException; import java.io.StringReader; import java.nio.file.Path; -import java.security.AccessController; -import java.security.PrivilegedAction; import java.util.ArrayList; import java.util.Collections; import java.util.HashSet; @@ -375,26 +376,88 @@ public void testNumDocs() throws IOException { dir.close(); } - @SuppressForbidden(reason = "tests readAnySegmentInfos with a feature flag enabled") - public void testReadAnySegmentInfos() throws IOException { + /** + * Tests whether old segments are readable and queryable based on the data documented + * in the README here. + * + * @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); - try (dir) { - // The standard API will throw an exception - expectThrows(IndexFormatTooOldException.class, () -> Lucene.readSegmentInfos(dir)); - // set the feature flag for extended backward compatibility - AccessController.doPrivileged( - (PrivilegedAction) () -> System.setProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC, "true") - ); - SegmentInfos si = Lucene.readAnySegmentInfos(dir); - assertEquals(1, Lucene.getNumDocs(si)); - } finally { - AccessController.doPrivileged( - (PrivilegedAction) () -> System.clearProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC) - ); + // The standard API will throw an exception + expectThrows(IndexFormatTooOldException.class, () -> Lucene.readSegmentInfos(dir)); + 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(); + } + + /** + * Since the implementation in {@link Lucene#readSegmentInfosExtendedCompatibility(Directory, Version)} + * is a workaround, this test verifies that the response from this method is equivalent to + * {@link Lucene#readSegmentInfos(Directory)} if the version is N-1 + */ + public void testReadSegmentInfosExtendedCompatibilityBaseCase() throws IOException { + MockDirectoryWrapper dir = newMockDirectory(); + IndexWriterConfig iwc = newIndexWriterConfig(); + IndexWriter writer = new IndexWriter(dir, iwc); + Document doc = new Document(); + doc.add(new TextField("id", "1", random().nextBoolean() ? Field.Store.YES : Field.Store.NO)); + writer.addDocument(doc); + writer.commit(); + SegmentInfos expectedSI = Lucene.readSegmentInfos(dir); + SegmentInfos actualSI = Lucene.readSegmentInfosExtendedCompatibility(dir, Version.CURRENT); + assertEquals(Lucene.getNumDocs(expectedSI), Lucene.getNumDocs(actualSI)); + assertEquals(expectedSI.getGeneration(), actualSI.getGeneration()); + assertEquals(expectedSI.getSegmentsFileName(), actualSI.getSegmentsFileName()); + assertEquals(expectedSI.getVersion(), actualSI.getVersion()); + assertEquals(expectedSI.getCommitLuceneVersion(), actualSI.getCommitLuceneVersion()); + assertEquals(expectedSI.getMinSegmentLuceneVersion(), actualSI.getMinSegmentLuceneVersion()); + assertEquals(expectedSI.getIndexCreatedVersionMajor(), actualSI.getIndexCreatedVersionMajor()); + assertEquals(expectedSI.getUserData(), actualSI.getUserData()); + + int numDocsToIndex = randomIntBetween(10, 50); + List deleteTerms = new ArrayList<>(); + for (int i = 0; i < numDocsToIndex; i++) { + doc = new Document(); + doc.add(new TextField("id", "doc_" + i, random().nextBoolean() ? Field.Store.YES : Field.Store.NO)); + deleteTerms.add(new Term("id", "doc_" + i)); + writer.addDocument(doc); } + int numDocsToDelete = randomIntBetween(0, numDocsToIndex); + Collections.shuffle(deleteTerms, random()); + for (int i = 0; i < numDocsToDelete; i++) { + Term remove = deleteTerms.remove(0); + writer.deleteDocuments(remove); + } + writer.commit(); + expectedSI = Lucene.readSegmentInfos(dir); + actualSI = Lucene.readSegmentInfosExtendedCompatibility(dir, Version.CURRENT); + assertEquals(Lucene.getNumDocs(expectedSI), Lucene.getNumDocs(actualSI)); + assertEquals(expectedSI.getGeneration(), actualSI.getGeneration()); + assertEquals(expectedSI.getSegmentsFileName(), actualSI.getSegmentsFileName()); + assertEquals(expectedSI.getVersion(), actualSI.getVersion()); + assertEquals(expectedSI.getCommitLuceneVersion(), actualSI.getCommitLuceneVersion()); + assertEquals(expectedSI.getMinSegmentLuceneVersion(), actualSI.getMinSegmentLuceneVersion()); + assertEquals(expectedSI.getIndexCreatedVersionMajor(), actualSI.getIndexCreatedVersionMajor()); + assertEquals(expectedSI.getUserData(), actualSI.getUserData()); + writer.close(); + dir.close(); } public void testCount() throws Exception { diff --git a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java index de5ef8851ae80..1938d0baaa29b 100644 --- a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java +++ b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java @@ -32,6 +32,7 @@ package org.opensearch.index; +import org.opensearch.LegacyESVersion; import org.opensearch.Version; import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.common.settings.AbstractScopedSettings; @@ -943,4 +944,47 @@ public void testSetRemoteRepositoryFailsWhenEmptyString() { ); assertEquals("Setting index.remote_store.repository should be provided with non-empty repository ID", iae.getMessage()); } + + public void testExtendedCompatibilityVersionForRemoteSnapshot() { + int version = randomIntBetween(1000000, 6999999); + Version expected = LegacyESVersion.fromId(version); + IndexMetadata metadata = newIndexMeta( + "index", + Settings.builder() + .put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT) + .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) + .put(IndexSettings.SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.getKey(), version) + .build() + ); + IndexSettings settings = new IndexSettings(metadata, Settings.EMPTY); + assertTrue(settings.isRemoteSnapshot()); + assertEquals(expected, settings.getExtendedCompatibilitySnapshotVersion()); + } + + public void testExtendedCompatibilityVersionForNonRemoteSnapshot() { + IndexMetadata metadata = newIndexMeta( + "index", + Settings.builder() + .put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT) + .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.FS.getSettingsKey()) + .put(IndexSettings.SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.getKey(), 99) + .build() + ); + IndexSettings settings = new IndexSettings(metadata, Settings.EMPTY); + assertFalse(settings.isRemoteSnapshot()); + assertNull(settings.getExtendedCompatibilitySnapshotVersion()); + } + + public void testExtendedCompatibilityVersionMissingKey() { + IndexMetadata metadata = newIndexMeta( + "index", + Settings.builder() + .put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT) + .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) + .build() + ); + IndexSettings settings = new IndexSettings(metadata, Settings.EMPTY); + assertTrue(settings.isRemoteSnapshot()); + assertNull(settings.getExtendedCompatibilitySnapshotVersion()); + } } diff --git a/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java b/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java index c609ae4ea36e8..b89316791ff48 100644 --- a/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java +++ b/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java @@ -33,14 +33,12 @@ import org.apache.lucene.index.DirectoryReader; import org.apache.lucene.index.IndexReader; -import org.apache.lucene.tests.store.BaseDirectoryWrapper; 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.common.SuppressForbidden; import org.opensearch.common.bytes.BytesArray; import org.opensearch.common.lucene.index.OpenSearchDirectoryReader; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.internal.io.IOUtils; import org.opensearch.index.mapper.ParsedDocument; import org.opensearch.index.seqno.SeqNoStats; @@ -50,8 +48,6 @@ import java.io.IOException; import java.nio.file.Path; -import java.security.AccessController; -import java.security.PrivilegedAction; import java.util.List; import java.util.concurrent.atomic.AtomicLong; import java.util.function.Function; @@ -235,7 +231,6 @@ public void testReadOnly() throws IOException { } } - @SuppressForbidden(reason = "tests creation of a ReadOnlyEngine with a feature flag enabled") public void testReadOldIndices() throws IOException { IOUtils.close(engine, store); // The index has one document in it, so the checkpoint cannot be NO_OPS_PERFORMED @@ -243,22 +238,22 @@ public void testReadOldIndices() throws IOException { final String pathToTestIndex = "/indices/bwc/testIndex-6.3.0.zip"; Path tmp = createTempDir(); TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp); - try (BaseDirectoryWrapper dir = newFSDirectory(tmp); Store store = createStore(dir)) { - // set the feature flag for extended backward compatibility - AccessController.doPrivileged( - (PrivilegedAction) () -> System.setProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC, "true") - ); - EngineConfig config = config(defaultSettings, store, createTempDir(), newMergePolicy(), null, null, globalCheckpoint::get); - try ( - ReadOnlyEngine readOnlyEngine = new ReadOnlyEngine(config, null, new TranslogStats(), true, Function.identity(), true, true) - ) { - assertVisibleCount(readOnlyEngine, 1, false); - } - } finally { - AccessController.doPrivileged( - (PrivilegedAction) () -> System.clearProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_BWC) - ); + 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); } + store.close(); } /** diff --git a/server/src/test/java/org/opensearch/index/store/StoreTests.java b/server/src/test/java/org/opensearch/index/store/StoreTests.java index dc6cf4c187f61..abf9847c2a261 100644 --- a/server/src/test/java/org/opensearch/index/store/StoreTests.java +++ b/server/src/test/java/org/opensearch/index/store/StoreTests.java @@ -66,6 +66,7 @@ import org.apache.lucene.util.Version; import org.hamcrest.Matchers; import org.opensearch.ExceptionsHelper; +import org.opensearch.LegacyESVersion; import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.common.UUIDs; import org.opensearch.common.io.stream.InputStreamStreamInput; @@ -76,6 +77,7 @@ import org.opensearch.core.internal.io.IOUtils; import org.opensearch.env.ShardLock; import org.opensearch.index.Index; +import org.opensearch.index.IndexModule; import org.opensearch.index.IndexSettings; import org.opensearch.index.engine.Engine; import org.opensearch.index.seqno.ReplicationTracker; @@ -1257,6 +1259,63 @@ public void testSegmentReplicationDiff() { assertTrue(diff.identical.isEmpty()); } + public void testReadSegmentsFromOldIndices() throws IOException { + int version = 6030099; + int expectedIndexCreatedVersionMajor = LegacyESVersion.fromId(version).luceneVersion.major; + final String pathToTestIndex = "/indices/bwc/testIndex-6.3.0.zip"; + Path tmp = createTempDir(); + TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp); + final ShardId shardId = new ShardId("index", "_na_", 1); + 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()) + .put(IndexSettings.SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.getKey(), String.valueOf(version)) + .build() + ); + Store store = new Store(shardId, indexSettings, StoreTests.newMockFSDirectory(tmp), new DummyShardLock(shardId)); + assertEquals(expectedIndexCreatedVersionMajor, store.readLastCommittedSegmentsInfo().getIndexCreatedVersionMajor()); + store.close(); + } + + public void testReadSegmentsFromOldIndicesFailure() throws IOException { + final String pathToTestIndex = "/indices/bwc/testIndex-6.3.0.zip"; + final ShardId shardId = new ShardId("index", "_na_", 1); + ArrayList testSettingsList = new ArrayList<>(); + // non-snapshot use-case + testSettingsList.add( + Settings.builder() + .put(IndexMetadata.SETTING_VERSION_CREATED, org.opensearch.Version.CURRENT) + .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.FS.getSettingsKey()) + .put(IndexSettings.SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.getKey(), String.valueOf(6030099)) + .build() + ); + // remote snapshot, but without the min-version key + testSettingsList.add( + Settings.builder() + .put(IndexMetadata.SETTING_VERSION_CREATED, org.opensearch.Version.CURRENT) + .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) + .build() + ); + // remote snapshot, but min-version mismatch + testSettingsList.add( + Settings.builder() + .put(IndexMetadata.SETTING_VERSION_CREATED, org.opensearch.Version.CURRENT) + .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) + .put(IndexSettings.SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.getKey(), String.valueOf(2000099 ^ org.opensearch.Version.MASK)) + .build() + ); + for (Settings testSettings : testSettingsList) { + Path tmp = createTempDir(); + TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp); + IndexSettings indexSettings = IndexSettingsModule.newIndexSettings("index", testSettings); + Store store = new Store(shardId, indexSettings, StoreTests.newMockFSDirectory(tmp), new DummyShardLock(shardId)); + assertThrows(IndexFormatTooOldException.class, store::readLastCommittedSegmentsInfo); + store.close(); + } + } + private void commitRandomDocs(Store store) throws IOException { IndexWriter writer = indexRandomDocs(store); writer.commit(); 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 new file mode 100644 index 0000000000000..31eb54e36271a --- /dev/null +++ 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 `/data/nodes/0/indices//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" +} +' +``` From e5d88808c2595885ef0250bc387065d4f4608d94 Mon Sep 17 00:00:00 2001 From: Kartik Ganesh Date: Mon, 19 Dec 2022 16:22:11 -0800 Subject: [PATCH 04/10] Incorporate PR comments from andrross Signed-off-by: Kartik Ganesh --- .../java/org/opensearch/index/IndexModule.java | 14 +++++++++++--- .../java/org/opensearch/index/IndexSettings.java | 6 ++++-- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/IndexModule.java b/server/src/main/java/org/opensearch/index/IndexModule.java index 9f7e3e9fb5eee..076d0b65a6113 100644 --- a/server/src/main/java/org/opensearch/index/IndexModule.java +++ b/server/src/main/java/org/opensearch/index/IndexModule.java @@ -459,11 +459,19 @@ public boolean match(String setting) { } /** - * Convenience method to check whether the given IndexSettings contains - * an {@link #INDEX_STORE_TYPE_SETTING} set to the value of this type. + * Convenience method to check whether the given {@link IndexSettings} + * object contains an {@link #INDEX_STORE_TYPE_SETTING} set to the value of this type. */ public boolean match(IndexSettings settings) { - return match(INDEX_STORE_TYPE_SETTING.get(settings.getSettings())); + return match(settings.getSettings()); + } + + /** + * Convenience method to check whether the given {@link Settings} + * object contains an {@link #INDEX_STORE_TYPE_SETTING} set to the value of this type. + */ + public boolean match(Settings settings) { + return match(INDEX_STORE_TYPE_SETTING.get(settings)); } } diff --git a/server/src/main/java/org/opensearch/index/IndexSettings.java b/server/src/main/java/org/opensearch/index/IndexSettings.java index bda5ddf9741a5..858394edfee54 100644 --- a/server/src/main/java/org/opensearch/index/IndexSettings.java +++ b/server/src/main/java/org/opensearch/index/IndexSettings.java @@ -60,6 +60,7 @@ import java.util.function.Function; import java.util.function.UnaryOperator; +import static org.opensearch.index.IndexModule.INDEX_STORE_TYPE_SETTING; import static org.opensearch.index.mapper.MapperService.INDEX_MAPPING_DEPTH_LIMIT_SETTING; import static org.opensearch.index.mapper.MapperService.INDEX_MAPPING_FIELD_NAME_LENGTH_LIMIT_SETTING; import static org.opensearch.index.mapper.MapperService.INDEX_MAPPING_NESTED_DOCS_LIMIT_SETTING; @@ -577,7 +578,7 @@ public final class IndexSettings { public static final Setting SEARCHABLE_SNAPSHOT_MINIMUM_VERSION = Setting.intSetting( "index.searchable_snapshot.minversion", - 0, + Version.V_EMPTY_ID, Property.IndexScope, Property.InternalIndex ); @@ -756,7 +757,8 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti isRemoteStoreEnabled = settings.getAsBoolean(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false); remoteStoreRepository = settings.get(IndexMetadata.SETTING_REMOTE_STORE_REPOSITORY); isRemoteTranslogStoreEnabled = settings.getAsBoolean(IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_ENABLED, false); - isRemoteSnapshot = IndexModule.Type.REMOTE_SNAPSHOT.match(this); + isRemoteSnapshot = IndexModule.Type.REMOTE_SNAPSHOT.match(this.settings); + if (isRemoteSnapshot) { Integer minVersion = settings.getAsInt(SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.getKey(), Version.V_EMPTY_ID); if (minVersion > Version.V_EMPTY_ID) { From a0eea5d8e1ceac5fd92bbc0ea9a65592cc4b36f8 Mon Sep 17 00:00:00 2001 From: Kartik Ganesh Date: Mon, 19 Dec 2022 19:38:44 -0800 Subject: [PATCH 05/10] Remove use of IndexSetting for minimum version for snapshots backwards compatibility Signed-off-by: Kartik Ganesh --- .../common/settings/IndexScopedSettings.java | 4 +- .../org/opensearch/index/IndexSettings.java | 16 ++-- .../directory/RemoteSnapshotDirectory.java | 5 ++ .../opensearch/snapshots/RestoreService.java | 19 ++--- .../opensearch/index/IndexSettingsTests.java | 40 ++++++---- .../opensearch/index/store/StoreTests.java | 77 +++++++++---------- 6 files changed, 79 insertions(+), 82 deletions(-) diff --git a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java index 900271431e6c2..079fc38415328 100644 --- a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java @@ -234,9 +234,7 @@ public final class IndexScopedSettings extends AbstractScopedSettings { IndexSettings.SEARCHABLE_SNAPSHOT_INDEX_ID, IndexSettings.SEARCHABLE_SNAPSHOT_ID_NAME, IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID - ), - FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY, - List.of(IndexSettings.SEARCHABLE_SNAPSHOT_MINIMUM_VERSION) + ) ); public static final IndexScopedSettings DEFAULT_SCOPED_SETTINGS = new IndexScopedSettings(Settings.EMPTY, BUILT_IN_INDEX_SETTINGS); diff --git a/server/src/main/java/org/opensearch/index/IndexSettings.java b/server/src/main/java/org/opensearch/index/IndexSettings.java index 858394edfee54..177635a2efd8e 100644 --- a/server/src/main/java/org/opensearch/index/IndexSettings.java +++ b/server/src/main/java/org/opensearch/index/IndexSettings.java @@ -46,6 +46,7 @@ import org.opensearch.common.unit.ByteSizeUnit; import org.opensearch.common.unit.ByteSizeValue; import org.opensearch.common.unit.TimeValue; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.index.translog.Translog; import org.opensearch.indices.replication.common.ReplicationType; import org.opensearch.ingest.IngestService; @@ -60,12 +61,13 @@ import java.util.function.Function; import java.util.function.UnaryOperator; -import static org.opensearch.index.IndexModule.INDEX_STORE_TYPE_SETTING; +import static org.opensearch.common.util.FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY; import static org.opensearch.index.mapper.MapperService.INDEX_MAPPING_DEPTH_LIMIT_SETTING; import static org.opensearch.index.mapper.MapperService.INDEX_MAPPING_FIELD_NAME_LENGTH_LIMIT_SETTING; import static org.opensearch.index.mapper.MapperService.INDEX_MAPPING_NESTED_DOCS_LIMIT_SETTING; import static org.opensearch.index.mapper.MapperService.INDEX_MAPPING_NESTED_FIELDS_LIMIT_SETTING; import static org.opensearch.index.mapper.MapperService.INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING; +import static org.opensearch.index.store.remote.directory.RemoteSnapshotDirectory.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION; /** * This class encapsulates all index level settings and handles settings updates. @@ -576,13 +578,6 @@ public final class IndexSettings { Property.InternalIndex ); - public static final Setting SEARCHABLE_SNAPSHOT_MINIMUM_VERSION = Setting.intSetting( - "index.searchable_snapshot.minversion", - Version.V_EMPTY_ID, - Property.IndexScope, - Property.InternalIndex - ); - private final Index index; private final Version version; private final Logger logger; @@ -760,9 +755,8 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti isRemoteSnapshot = IndexModule.Type.REMOTE_SNAPSHOT.match(this.settings); if (isRemoteSnapshot) { - Integer minVersion = settings.getAsInt(SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.getKey(), Version.V_EMPTY_ID); - if (minVersion > Version.V_EMPTY_ID) { - extendedCompatibilitySnapshotVersion = LegacyESVersion.fromId(minVersion); + if (FeatureFlags.isEnabled(SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY)) { + extendedCompatibilitySnapshotVersion = SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION; } } this.searchThrottled = INDEX_SEARCH_THROTTLED.get(settings); diff --git a/server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectory.java b/server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectory.java index 3a2749a6d325b..e723c831b213d 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectory.java @@ -22,6 +22,8 @@ import org.apache.lucene.store.IndexOutput; import org.apache.lucene.store.Lock; import org.apache.lucene.store.NoLockFactory; +import org.opensearch.LegacyESVersion; +import org.opensearch.Version; import org.opensearch.index.snapshots.blobstore.BlobStoreIndexShardSnapshot; import org.opensearch.index.store.remote.file.OnDemandBlockSnapshotIndexInput; import org.opensearch.index.store.remote.file.OnDemandVirtualFileSnapshotIndexInput; @@ -35,6 +37,9 @@ * @opensearch.internal */ public final class RemoteSnapshotDirectory extends Directory { + + public static final Version SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION = LegacyESVersion.fromId(6000099); + private static final String VIRTUAL_FILE_PREFIX = BlobStoreRepository.VIRTUAL_DATA_BLOB_PREFIX; private final Map fileInfoMap; diff --git a/server/src/main/java/org/opensearch/snapshots/RestoreService.java b/server/src/main/java/org/opensearch/snapshots/RestoreService.java index b3c6d5f84af81..7d9b15f2ef296 100644 --- a/server/src/main/java/org/opensearch/snapshots/RestoreService.java +++ b/server/src/main/java/org/opensearch/snapshots/RestoreService.java @@ -123,6 +123,7 @@ import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED; import static org.opensearch.common.util.FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY; import static org.opensearch.common.util.set.Sets.newHashSet; +import static org.opensearch.index.store.remote.directory.RemoteSnapshotDirectory.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION; import static org.opensearch.snapshots.SnapshotUtils.filterIndices; /** @@ -472,10 +473,8 @@ public ClusterState execute(ClusterState currentState) { ); final Version minIndexCompatibilityVersion; if (isSearchableSnapshot && isSearchableSnapshotsExtendedCompatibilityEnabled()) { - final int minVersion = IndexSettings.SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.get( - snapshotIndexMetadata.getSettings() - ); - minIndexCompatibilityVersion = LegacyESVersion.fromId(minVersion).minimumIndexCompatibilityVersion(); + minIndexCompatibilityVersion = SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION + .minimumIndexCompatibilityVersion(); } else { minIndexCompatibilityVersion = currentState.getNodes() .getMaxNodeVersion() @@ -1254,19 +1253,15 @@ public void applyClusterState(ClusterChangedEvent event) { } private static IndexMetadata addSnapshotToIndexSettings(IndexMetadata metadata, Snapshot snapshot, IndexId indexId) { - final Settings.Builder settingsBuilder = Settings.builder() + final Settings newSettings = Settings.builder() .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) .put(IndexSettings.SEARCHABLE_SNAPSHOT_REPOSITORY.getKey(), snapshot.getRepository()) .put(IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID.getKey(), snapshot.getSnapshotId().getUUID()) .put(IndexSettings.SEARCHABLE_SNAPSHOT_ID_NAME.getKey(), snapshot.getSnapshotId().getName()) .put(IndexSettings.SEARCHABLE_SNAPSHOT_INDEX_ID.getKey(), indexId.getId()) - .put(metadata.getSettings()); - // Check for extended backwards compatibility feature flag - if (isSearchableSnapshotsExtendedCompatibilityEnabled()) { - // The oldest snapshot version we can read is ES 6.0 - settingsBuilder.put(IndexSettings.SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.getKey(), 6000099); - } - return IndexMetadata.builder(metadata).settings(settingsBuilder.build()).build(); + .put(metadata.getSettings()) + .build(); + return IndexMetadata.builder(metadata).settings(newSettings).build(); } private static boolean isSearchableSnapshotsExtendedCompatibilityEnabled() { diff --git a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java index 1938d0baaa29b..74125485f8937 100644 --- a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java +++ b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java @@ -32,9 +32,9 @@ package org.opensearch.index; -import org.opensearch.LegacyESVersion; import org.opensearch.Version; import org.opensearch.cluster.metadata.IndexMetadata; +import org.opensearch.common.SuppressForbidden; import org.opensearch.common.settings.AbstractScopedSettings; import org.opensearch.common.settings.IndexScopedSettings; import org.opensearch.common.settings.Setting; @@ -42,11 +42,14 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.ByteSizeValue; import org.opensearch.common.unit.TimeValue; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.index.translog.Translog; import org.opensearch.indices.replication.common.ReplicationType; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.test.VersionUtils; +import java.security.AccessController; +import java.security.PrivilegedAction; import java.util.Arrays; import java.util.Collections; import java.util.HashSet; @@ -58,6 +61,7 @@ import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.core.StringContains.containsString; import static org.hamcrest.object.HasToString.hasToString; +import static org.opensearch.index.store.remote.directory.RemoteSnapshotDirectory.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION; public class IndexSettingsTests extends OpenSearchTestCase { @@ -945,20 +949,27 @@ public void testSetRemoteRepositoryFailsWhenEmptyString() { assertEquals("Setting index.remote_store.repository should be provided with non-empty repository ID", iae.getMessage()); } + @SuppressForbidden(reason = "sets the SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY feature flag") public void testExtendedCompatibilityVersionForRemoteSnapshot() { - int version = randomIntBetween(1000000, 6999999); - Version expected = LegacyESVersion.fromId(version); - IndexMetadata metadata = newIndexMeta( - "index", - Settings.builder() - .put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT) - .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) - .put(IndexSettings.SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.getKey(), version) - .build() - ); - IndexSettings settings = new IndexSettings(metadata, Settings.EMPTY); - assertTrue(settings.isRemoteSnapshot()); - assertEquals(expected, settings.getExtendedCompatibilitySnapshotVersion()); + try { + AccessController.doPrivileged( + (PrivilegedAction) () -> System.setProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY, "true") + ); + IndexMetadata metadata = newIndexMeta( + "index", + Settings.builder() + .put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT) + .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) + .build() + ); + IndexSettings settings = new IndexSettings(metadata, Settings.EMPTY); + assertTrue(settings.isRemoteSnapshot()); + assertEquals(SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION, settings.getExtendedCompatibilitySnapshotVersion()); + } finally { + AccessController.doPrivileged( + (PrivilegedAction) () -> System.clearProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY) + ); + } } public void testExtendedCompatibilityVersionForNonRemoteSnapshot() { @@ -967,7 +978,6 @@ public void testExtendedCompatibilityVersionForNonRemoteSnapshot() { Settings.builder() .put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT) .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.FS.getSettingsKey()) - .put(IndexSettings.SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.getKey(), 99) .build() ); IndexSettings settings = new IndexSettings(metadata, Settings.EMPTY); diff --git a/server/src/test/java/org/opensearch/index/store/StoreTests.java b/server/src/test/java/org/opensearch/index/store/StoreTests.java index abf9847c2a261..406797857d440 100644 --- a/server/src/test/java/org/opensearch/index/store/StoreTests.java +++ b/server/src/test/java/org/opensearch/index/store/StoreTests.java @@ -66,14 +66,15 @@ import org.apache.lucene.util.Version; import org.hamcrest.Matchers; import org.opensearch.ExceptionsHelper; -import org.opensearch.LegacyESVersion; import org.opensearch.cluster.metadata.IndexMetadata; +import org.opensearch.common.SuppressForbidden; import org.opensearch.common.UUIDs; import org.opensearch.common.io.stream.InputStreamStreamInput; import org.opensearch.common.io.stream.OutputStreamStreamOutput; import org.opensearch.common.lucene.Lucene; import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.internal.io.IOUtils; import org.opensearch.env.ShardLock; import org.opensearch.index.Index; @@ -95,6 +96,8 @@ import java.io.IOException; import java.nio.file.NoSuchFileException; import java.nio.file.Path; +import java.security.AccessController; +import java.security.PrivilegedAction; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -118,6 +121,7 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; +import static org.opensearch.index.store.remote.directory.RemoteSnapshotDirectory.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION; import static org.opensearch.test.VersionUtils.randomVersion; public class StoreTests extends OpenSearchTestCase { @@ -1259,61 +1263,52 @@ public void testSegmentReplicationDiff() { assertTrue(diff.identical.isEmpty()); } + @SuppressForbidden(reason = "sets the SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY feature flag") public void testReadSegmentsFromOldIndices() throws IOException { - int version = 6030099; - int expectedIndexCreatedVersionMajor = LegacyESVersion.fromId(version).luceneVersion.major; + int expectedIndexCreatedVersionMajor = SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION.luceneVersion.major; final String pathToTestIndex = "/indices/bwc/testIndex-6.3.0.zip"; Path tmp = createTempDir(); TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp); final ShardId shardId = new ShardId("index", "_na_", 1); - 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()) - .put(IndexSettings.SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.getKey(), String.valueOf(version)) - .build() - ); - Store store = new Store(shardId, indexSettings, StoreTests.newMockFSDirectory(tmp), new DummyShardLock(shardId)); - assertEquals(expectedIndexCreatedVersionMajor, store.readLastCommittedSegmentsInfo().getIndexCreatedVersionMajor()); - store.close(); + Store store = null; + try { + AccessController.doPrivileged( + (PrivilegedAction) () -> System.setProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY, "true") + ); + 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() + ); + store = new Store(shardId, indexSettings, StoreTests.newMockFSDirectory(tmp), new DummyShardLock(shardId)); + assertEquals(expectedIndexCreatedVersionMajor, store.readLastCommittedSegmentsInfo().getIndexCreatedVersionMajor()); + } finally { + AccessController.doPrivileged( + (PrivilegedAction) () -> System.clearProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY) + ); + if (store != null) { + store.close(); + } + } } public void testReadSegmentsFromOldIndicesFailure() throws IOException { final String pathToTestIndex = "/indices/bwc/testIndex-6.3.0.zip"; final ShardId shardId = new ShardId("index", "_na_", 1); - ArrayList testSettingsList = new ArrayList<>(); - // non-snapshot use-case - testSettingsList.add( + Path tmp = createTempDir(); + TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp); + 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.FS.getSettingsKey()) - .put(IndexSettings.SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.getKey(), String.valueOf(6030099)) - .build() - ); - // remote snapshot, but without the min-version key - testSettingsList.add( - Settings.builder() - .put(IndexMetadata.SETTING_VERSION_CREATED, org.opensearch.Version.CURRENT) - .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) .build() ); - // remote snapshot, but min-version mismatch - testSettingsList.add( - Settings.builder() - .put(IndexMetadata.SETTING_VERSION_CREATED, org.opensearch.Version.CURRENT) - .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) - .put(IndexSettings.SEARCHABLE_SNAPSHOT_MINIMUM_VERSION.getKey(), String.valueOf(2000099 ^ org.opensearch.Version.MASK)) - .build() - ); - for (Settings testSettings : testSettingsList) { - Path tmp = createTempDir(); - TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp); - IndexSettings indexSettings = IndexSettingsModule.newIndexSettings("index", testSettings); - Store store = new Store(shardId, indexSettings, StoreTests.newMockFSDirectory(tmp), new DummyShardLock(shardId)); - assertThrows(IndexFormatTooOldException.class, store::readLastCommittedSegmentsInfo); - store.close(); - } + Store store = new Store(shardId, indexSettings, StoreTests.newMockFSDirectory(tmp), new DummyShardLock(shardId)); + assertThrows(IndexFormatTooOldException.class, store::readLastCommittedSegmentsInfo); + store.close(); } private void commitRandomDocs(Store store) throws IOException { From 440b06651e7f409d846516b42b829103f2145ab6 Mon Sep 17 00:00:00 2001 From: Kartik Ganesh Date: Tue, 20 Dec 2022 12:39:13 -0800 Subject: [PATCH 06/10] Moved ES 6.3.0 test data to a subdirectory This change also includes an update to the file name to clarify that it is an ES index, and changing the associated markdown file name to just README.md. All tests that reference this ZIP file have corresponding changes to the path they reference. Signed-off-by: Kartik Ganesh --- .../java/org/opensearch/index/IndexSettings.java | 6 ++---- .../org/opensearch/common/lucene/LuceneTests.java | 4 ++-- .../index/engine/ReadOnlyEngineTests.java | 2 +- .../java/org/opensearch/index/store/StoreTests.java | 4 ++-- .../bwc/{testIndex-6.3.0.md => es-6.3.0/README.md} | 2 +- .../testIndex-es-6.3.0.zip} | Bin 6 files changed, 8 insertions(+), 10 deletions(-) rename server/src/test/resources/indices/bwc/{testIndex-6.3.0.md => es-6.3.0/README.md} (97%) rename server/src/test/resources/indices/bwc/{testIndex-6.3.0.zip => es-6.3.0/testIndex-es-6.3.0.zip} (100%) diff --git a/server/src/main/java/org/opensearch/index/IndexSettings.java b/server/src/main/java/org/opensearch/index/IndexSettings.java index 177635a2efd8e..38a3817fd6f9c 100644 --- a/server/src/main/java/org/opensearch/index/IndexSettings.java +++ b/server/src/main/java/org/opensearch/index/IndexSettings.java @@ -754,10 +754,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.settings); - if (isRemoteSnapshot) { - if (FeatureFlags.isEnabled(SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY)) { - extendedCompatibilitySnapshotVersion = SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION; - } + if (isRemoteSnapshot && FeatureFlags.isEnabled(SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY)) { + extendedCompatibilitySnapshotVersion = SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION; } this.searchThrottled = INDEX_SEARCH_THROTTLED.get(settings); this.queryStringLenient = QUERY_STRING_LENIENT_SETTING.get(settings); diff --git a/server/src/test/java/org/opensearch/common/lucene/LuceneTests.java b/server/src/test/java/org/opensearch/common/lucene/LuceneTests.java index d4cf1fb1836e6..c3955690c62b7 100644 --- a/server/src/test/java/org/opensearch/common/lucene/LuceneTests.java +++ b/server/src/test/java/org/opensearch/common/lucene/LuceneTests.java @@ -378,12 +378,12 @@ public void testNumDocs() throws IOException { /** * Tests whether old segments are readable and queryable based on the data documented - * in the README here. + * in the README here. * * @throws IOException */ public void testReadSegmentInfosExtendedCompatibility() throws IOException { - final String pathToTestIndex = "/indices/bwc/testIndex-6.3.0.zip"; + final String pathToTestIndex = "/indices/bwc/es-6.3.0/testIndex-es-6.3.0.zip"; final Version minVersion = LegacyESVersion.fromId(6000099); Path tmp = createTempDir(); TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp); diff --git a/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java b/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java index b89316791ff48..519cb50b6a2a2 100644 --- a/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java +++ b/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java @@ -235,7 +235,7 @@ public void testReadOldIndices() 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); - final String pathToTestIndex = "/indices/bwc/testIndex-6.3.0.zip"; + final String pathToTestIndex = "/indices/bwc/es-6.3.0/testIndex-es-6.3.0.zip"; Path tmp = createTempDir(); TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp); Store store = createStore(newFSDirectory(tmp)); diff --git a/server/src/test/java/org/opensearch/index/store/StoreTests.java b/server/src/test/java/org/opensearch/index/store/StoreTests.java index 406797857d440..c24a1c6805962 100644 --- a/server/src/test/java/org/opensearch/index/store/StoreTests.java +++ b/server/src/test/java/org/opensearch/index/store/StoreTests.java @@ -1266,7 +1266,7 @@ public void testSegmentReplicationDiff() { @SuppressForbidden(reason = "sets the SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY feature flag") public void testReadSegmentsFromOldIndices() throws IOException { int expectedIndexCreatedVersionMajor = SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION.luceneVersion.major; - final String pathToTestIndex = "/indices/bwc/testIndex-6.3.0.zip"; + final String pathToTestIndex = "/indices/bwc/es-6.3.0/testIndex-es-6.3.0.zip"; Path tmp = createTempDir(); TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp); final ShardId shardId = new ShardId("index", "_na_", 1); @@ -1295,7 +1295,7 @@ public void testReadSegmentsFromOldIndices() throws IOException { } public void testReadSegmentsFromOldIndicesFailure() throws IOException { - final String pathToTestIndex = "/indices/bwc/testIndex-6.3.0.zip"; + final String pathToTestIndex = "/indices/bwc/es-6.3.0/testIndex-es-6.3.0.zip"; final ShardId shardId = new ShardId("index", "_na_", 1); Path tmp = createTempDir(); TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp); diff --git a/server/src/test/resources/indices/bwc/testIndex-6.3.0.md b/server/src/test/resources/indices/bwc/es-6.3.0/README.md similarity index 97% rename from server/src/test/resources/indices/bwc/testIndex-6.3.0.md rename to server/src/test/resources/indices/bwc/es-6.3.0/README.md index 31eb54e36271a..a9f969475aaad 100644 --- a/server/src/test/resources/indices/bwc/testIndex-6.3.0.md +++ b/server/src/test/resources/indices/bwc/es-6.3.0/README.md @@ -1,4 +1,4 @@ -# README for _testIndex-6.3.0.zip_ +# README for _testIndex-es-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, diff --git a/server/src/test/resources/indices/bwc/testIndex-6.3.0.zip b/server/src/test/resources/indices/bwc/es-6.3.0/testIndex-es-6.3.0.zip similarity index 100% rename from server/src/test/resources/indices/bwc/testIndex-6.3.0.zip rename to server/src/test/resources/indices/bwc/es-6.3.0/testIndex-es-6.3.0.zip From 73a142925543f8bd7ff38deb1c04518ae47d3baa Mon Sep 17 00:00:00 2001 From: Kartik Ganesh Date: Thu, 22 Dec 2022 12:01:54 -0800 Subject: [PATCH 07/10] Update unit tests to use try-with-resources Signed-off-by: Kartik Ganesh --- .../opensearch/common/lucene/LuceneTests.java | 38 ++++++++++--------- .../index/engine/ReadOnlyEngineTests.java | 30 +++++++-------- 2 files changed, 35 insertions(+), 33 deletions(-) diff --git a/server/src/test/java/org/opensearch/common/lucene/LuceneTests.java b/server/src/test/java/org/opensearch/common/lucene/LuceneTests.java index c3955690c62b7..bcbe1b0320f49 100644 --- a/server/src/test/java/org/opensearch/common/lucene/LuceneTests.java +++ b/server/src/test/java/org/opensearch/common/lucene/LuceneTests.java @@ -387,24 +387,26 @@ public void testReadSegmentInfosExtendedCompatibility() throws IOException { 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.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(); + try (MockDirectoryWrapper dir = newMockFSDirectory(tmp)) { + // The standard API will throw an exception + expectThrows(IndexFormatTooOldException.class, () -> Lucene.readSegmentInfos(dir)); + SegmentInfos si = Lucene.readSegmentInfosExtendedCompatibility(dir, minVersion); + assertEquals(1, Lucene.getNumDocs(si)); + IndexCommit indexCommit = Lucene.getIndexCommit(si, dir); + // uses the "expert" Lucene API + try ( + 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))); + } + } } /** diff --git a/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java b/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java index 519cb50b6a2a2..872b027e7a0d2 100644 --- a/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java +++ b/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java @@ -238,22 +238,22 @@ public void testReadOldIndices() throws IOException { final String pathToTestIndex = "/indices/bwc/es-6.3.0/testIndex-es-6.3.0.zip"; Path tmp = createTempDir(); TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp); - 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 (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); + } } - store.close(); } /** From 2006683c23959c5fc166be2235a278ac02860fb2 Mon Sep 17 00:00:00 2001 From: Kartik Ganesh Date: Fri, 23 Dec 2022 21:32:54 -0800 Subject: [PATCH 08/10] Added FeatureFlagSetter helper class Also refactored unit test classes to use the helper class. Signed-off-by: Kartik Ganesh --- .../common/settings/SettingsModuleTests.java | 73 ++++++++++--------- .../common/util/FeatureFlagTests.java | 33 +++------ .../extensions/ExtensionsManagerTests.java | 7 +- .../opensearch/index/IndexSettingsTests.java | 14 +--- .../opensearch/index/store/StoreTests.java | 14 +--- .../opensearch/test/FeatureFlagSetter.java | 39 ++++++++++ 6 files changed, 98 insertions(+), 82 deletions(-) create mode 100644 test/framework/src/main/java/org/opensearch/test/FeatureFlagSetter.java diff --git a/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java b/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java index 8b53e5fe51635..b145ec0d79311 100644 --- a/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java +++ b/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java @@ -34,8 +34,9 @@ import org.opensearch.common.inject.ModuleTestCase; import org.opensearch.common.settings.Setting.Property; -import org.opensearch.common.util.FeatureFlagTests; import org.hamcrest.Matchers; +import org.opensearch.common.util.FeatureFlags; +import org.opensearch.test.FeatureFlagSetter; import java.util.Arrays; @@ -239,46 +240,48 @@ public void testOldMaxClauseCountSetting() { ); } - public void testDynamicNodeSettingsRegistration() { - FeatureFlagTests.enableFeature(); - Settings settings = Settings.builder().put("some.custom.setting", "2.0").build(); - SettingsModule module = new SettingsModule(settings, Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope)); - assertNotNull(module.getClusterSettings().get("some.custom.setting")); - // For unregistered setting the value is expected to be null - assertNull(module.getClusterSettings().get("some.custom.setting2")); - assertInstanceBinding(module, Settings.class, (s) -> s == settings); + public void testDynamicNodeSettingsRegistration() throws Exception { + try (FeatureFlagSetter f = FeatureFlagSetter.set(FeatureFlags.EXTENSIONS)) { + Settings settings = Settings.builder().put("some.custom.setting", "2.0").build(); + SettingsModule module = new SettingsModule(settings, Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope)); + assertNotNull(module.getClusterSettings().get("some.custom.setting")); + // For unregistered setting the value is expected to be null + assertNull(module.getClusterSettings().get("some.custom.setting2")); + assertInstanceBinding(module, Settings.class, (s) -> s == settings); - assertTrue(module.registerDynamicSetting(Setting.floatSetting("some.custom.setting2", 1.0f, Property.NodeScope))); - assertNotNull(module.getClusterSettings().get("some.custom.setting2")); - // verify if some.custom.setting still exists - assertNotNull(module.getClusterSettings().get("some.custom.setting")); + assertTrue(module.registerDynamicSetting(Setting.floatSetting("some.custom.setting2", 1.0f, Property.NodeScope))); + assertNotNull(module.getClusterSettings().get("some.custom.setting2")); + // verify if some.custom.setting still exists + assertNotNull(module.getClusterSettings().get("some.custom.setting")); - // verify exception is thrown when setting registration fails - expectThrows( - SettingsException.class, - () -> module.registerDynamicSetting(Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope)) - ); + // verify exception is thrown when setting registration fails + expectThrows( + SettingsException.class, + () -> module.registerDynamicSetting(Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope)) + ); + } } - public void testDynamicIndexSettingsRegistration() { - FeatureFlagTests.enableFeature(); - Settings settings = Settings.builder().put("some.custom.setting", "2.0").build(); - SettingsModule module = new SettingsModule(settings, Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope)); - assertNotNull(module.getClusterSettings().get("some.custom.setting")); - // For unregistered setting the value is expected to be null - assertNull(module.getIndexScopedSettings().get("index.custom.setting2")); - assertInstanceBinding(module, Settings.class, (s) -> s == settings); + public void testDynamicIndexSettingsRegistration() throws Exception { + try (FeatureFlagSetter f = FeatureFlagSetter.set(FeatureFlags.EXTENSIONS)) { + Settings settings = Settings.builder().put("some.custom.setting", "2.0").build(); + SettingsModule module = new SettingsModule(settings, Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope)); + assertNotNull(module.getClusterSettings().get("some.custom.setting")); + // For unregistered setting the value is expected to be null + assertNull(module.getIndexScopedSettings().get("index.custom.setting2")); + assertInstanceBinding(module, Settings.class, (s) -> s == settings); - assertTrue(module.registerDynamicSetting(Setting.floatSetting("index.custom.setting2", 1.0f, Property.IndexScope))); - assertNotNull(module.getIndexScopedSettings().get("index.custom.setting2")); + assertTrue(module.registerDynamicSetting(Setting.floatSetting("index.custom.setting2", 1.0f, Property.IndexScope))); + assertNotNull(module.getIndexScopedSettings().get("index.custom.setting2")); - // verify if some.custom.setting still exists - assertNotNull(module.getClusterSettings().get("some.custom.setting")); + // verify if some.custom.setting still exists + assertNotNull(module.getClusterSettings().get("some.custom.setting")); - // verify exception is thrown when setting registration fails - expectThrows( - SettingsException.class, - () -> module.registerDynamicSetting(Setting.floatSetting("index.custom.setting2", 1.0f, Property.IndexScope)) - ); + // verify exception is thrown when setting registration fails + expectThrows( + SettingsException.class, + () -> module.registerDynamicSetting(Setting.floatSetting("index.custom.setting2", 1.0f, Property.IndexScope)) + ); + } } } diff --git a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java index 05ede515e042c..ca16efdf11d7d 100644 --- a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java +++ b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java @@ -8,31 +8,23 @@ package org.opensearch.common.util; -import org.junit.BeforeClass; -import org.opensearch.common.SuppressForbidden; +import org.opensearch.test.FeatureFlagSetter; import org.opensearch.test.OpenSearchTestCase; -import java.security.AccessController; -import java.security.PrivilegedAction; - public class FeatureFlagTests extends OpenSearchTestCase { - @SuppressForbidden(reason = "sets the feature flag") - @BeforeClass - public static void enableFeature() { - AccessController.doPrivileged((PrivilegedAction) () -> System.setProperty(FeatureFlags.REPLICATION_TYPE, "true")); - AccessController.doPrivileged((PrivilegedAction) () -> System.setProperty(FeatureFlags.REMOTE_STORE, "true")); - AccessController.doPrivileged((PrivilegedAction) () -> System.setProperty(FeatureFlags.EXTENSIONS, "true")); - } + private final String FLAG_PREFIX = "opensearch.experimental.feature."; - public void testReplicationTypeFeatureFlag() { - String replicationTypeFlag = FeatureFlags.REPLICATION_TYPE; - assertNotNull(System.getProperty(replicationTypeFlag)); - assertTrue(FeatureFlags.isEnabled(replicationTypeFlag)); + public void testFeatureFlagSet() throws Exception { + final String testFlag = FLAG_PREFIX + "testFlag"; + try (FeatureFlagSetter f = FeatureFlagSetter.set(testFlag)) { + assertNotNull(System.getProperty(testFlag)); + assertTrue(FeatureFlags.isEnabled(testFlag)); + } } public void testMissingFeatureFlag() { - String testFlag = "missingFeatureFlag"; + final String testFlag = FLAG_PREFIX + "testFlag"; assertNull(System.getProperty(testFlag)); assertFalse(FeatureFlags.isEnabled(testFlag)); } @@ -42,11 +34,4 @@ public void testNonBooleanFeatureFlag() { assertNotNull(System.getProperty(javaVersionProperty)); assertFalse(FeatureFlags.isEnabled(javaVersionProperty)); } - - public void testRemoteStoreFeatureFlag() { - String remoteStoreFlag = FeatureFlags.REMOTE_STORE; - assertNotNull(System.getProperty(remoteStoreFlag)); - assertTrue(FeatureFlags.isEnabled(remoteStoreFlag)); - } - } diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index 47820ee739c49..af51dfa49ed6a 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -60,7 +60,7 @@ import org.opensearch.common.settings.WriteableSetting.SettingType; import org.opensearch.common.settings.SettingsModule; import org.opensearch.common.transport.TransportAddress; -import org.opensearch.common.util.FeatureFlagTests; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.PageCacheRecycler; import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.env.Environment; @@ -75,6 +75,7 @@ import org.opensearch.indices.breaker.NoneCircuitBreakerService; import org.opensearch.plugins.PluginInfo; import org.opensearch.rest.RestController; +import org.opensearch.test.FeatureFlagSetter; import org.opensearch.test.IndexSettingsModule; import org.opensearch.test.MockLogAppender; import org.opensearch.test.OpenSearchTestCase; @@ -90,6 +91,7 @@ public class ExtensionsManagerTests extends OpenSearchTestCase { + private FeatureFlagSetter featureFlagSetter; private TransportService transportService; private RestController restController; private SettingsModule settingsModule; @@ -137,7 +139,7 @@ public class ExtensionsManagerTests extends OpenSearchTestCase { @Before public void setup() throws Exception { - FeatureFlagTests.enableFeature(); + featureFlagSetter = FeatureFlagSetter.set(FeatureFlags.EXTENSIONS); Settings settings = Settings.builder().put("cluster.name", "test").build(); transport = new MockNioTransport( settings, @@ -207,6 +209,7 @@ public void tearDown() throws Exception { transportService.close(); client.close(); ThreadPool.terminate(threadPool, 30, TimeUnit.SECONDS); + featureFlagSetter.close(); } public void testDiscover() throws Exception { diff --git a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java index 74125485f8937..ebfec00b97d2d 100644 --- a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java +++ b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java @@ -45,11 +45,10 @@ import org.opensearch.common.util.FeatureFlags; import org.opensearch.index.translog.Translog; import org.opensearch.indices.replication.common.ReplicationType; +import org.opensearch.test.FeatureFlagSetter; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.test.VersionUtils; -import java.security.AccessController; -import java.security.PrivilegedAction; import java.util.Arrays; import java.util.Collections; import java.util.HashSet; @@ -950,11 +949,8 @@ public void testSetRemoteRepositoryFailsWhenEmptyString() { } @SuppressForbidden(reason = "sets the SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY feature flag") - public void testExtendedCompatibilityVersionForRemoteSnapshot() { - try { - AccessController.doPrivileged( - (PrivilegedAction) () -> System.setProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY, "true") - ); + public void testExtendedCompatibilityVersionForRemoteSnapshot() throws Exception { + try (FeatureFlagSetter f = FeatureFlagSetter.set(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY)) { IndexMetadata metadata = newIndexMeta( "index", Settings.builder() @@ -965,10 +961,6 @@ public void testExtendedCompatibilityVersionForRemoteSnapshot() { IndexSettings settings = new IndexSettings(metadata, Settings.EMPTY); assertTrue(settings.isRemoteSnapshot()); assertEquals(SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION, settings.getExtendedCompatibilitySnapshotVersion()); - } finally { - AccessController.doPrivileged( - (PrivilegedAction) () -> System.clearProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY) - ); } } diff --git a/server/src/test/java/org/opensearch/index/store/StoreTests.java b/server/src/test/java/org/opensearch/index/store/StoreTests.java index c24a1c6805962..7f5340096ab86 100644 --- a/server/src/test/java/org/opensearch/index/store/StoreTests.java +++ b/server/src/test/java/org/opensearch/index/store/StoreTests.java @@ -87,6 +87,7 @@ import org.opensearch.indices.replication.common.ReplicationType; import org.opensearch.indices.store.TransportNodesListShardStoreMetadata; import org.opensearch.test.DummyShardLock; +import org.opensearch.test.FeatureFlagSetter; import org.opensearch.test.IndexSettingsModule; import org.opensearch.test.OpenSearchTestCase; @@ -96,8 +97,6 @@ import java.io.IOException; import java.nio.file.NoSuchFileException; import java.nio.file.Path; -import java.security.AccessController; -import java.security.PrivilegedAction; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -1264,17 +1263,15 @@ public void testSegmentReplicationDiff() { } @SuppressForbidden(reason = "sets the SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY feature flag") - public void testReadSegmentsFromOldIndices() throws IOException { + public void testReadSegmentsFromOldIndices() throws Exception { int expectedIndexCreatedVersionMajor = SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION.luceneVersion.major; final String pathToTestIndex = "/indices/bwc/es-6.3.0/testIndex-es-6.3.0.zip"; Path tmp = createTempDir(); TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp); final ShardId shardId = new ShardId("index", "_na_", 1); Store store = null; - try { - AccessController.doPrivileged( - (PrivilegedAction) () -> System.setProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY, "true") - ); + + try (FeatureFlagSetter f = FeatureFlagSetter.set(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY)) { IndexSettings indexSettings = IndexSettingsModule.newIndexSettings( "index", Settings.builder() @@ -1285,9 +1282,6 @@ public void testReadSegmentsFromOldIndices() throws IOException { store = new Store(shardId, indexSettings, StoreTests.newMockFSDirectory(tmp), new DummyShardLock(shardId)); assertEquals(expectedIndexCreatedVersionMajor, store.readLastCommittedSegmentsInfo().getIndexCreatedVersionMajor()); } finally { - AccessController.doPrivileged( - (PrivilegedAction) () -> System.clearProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY) - ); if (store != null) { store.close(); } diff --git a/test/framework/src/main/java/org/opensearch/test/FeatureFlagSetter.java b/test/framework/src/main/java/org/opensearch/test/FeatureFlagSetter.java new file mode 100644 index 0000000000000..26e884e707964 --- /dev/null +++ b/test/framework/src/main/java/org/opensearch/test/FeatureFlagSetter.java @@ -0,0 +1,39 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.test; + +import org.opensearch.common.SuppressForbidden; + +import java.security.AccessController; +import java.security.PrivilegedAction; + +/** + * Helper class that wraps the lifecycle of setting and finally clearing of + * a {@link org.opensearch.common.util.FeatureFlags} string in an {@link AutoCloseable}. + */ +public class FeatureFlagSetter implements AutoCloseable { + + private final String flag; + + private FeatureFlagSetter(String flag) { + this.flag = flag; + } + + @SuppressForbidden(reason = "Enables setting of feature flags") + public static final FeatureFlagSetter set(String flag) { + AccessController.doPrivileged((PrivilegedAction) () -> System.setProperty(flag, "true")); + return new FeatureFlagSetter(flag); + } + + @SuppressForbidden(reason = "Clears the set feature flag on close") + @Override + public void close() throws Exception { + AccessController.doPrivileged((PrivilegedAction) () -> System.clearProperty(this.flag)); + } +} From 433924462f69bbc6fe4282c4d975ed4154cf6755 Mon Sep 17 00:00:00 2001 From: Kartik Ganesh Date: Tue, 3 Jan 2023 13:15:04 -0800 Subject: [PATCH 09/10] Incorporating PR feedback from @mch2 Signed-off-by: Kartik Ganesh --- .../org/opensearch/index/IndexSettings.java | 2 + .../index/engine/ReadOnlyEngine.java | 31 ++--------- .../opensearch/indices/IndicesService.java | 10 +--- .../index/engine/ReadOnlyEngineTests.java | 54 ++++++++++++++----- 4 files changed, 47 insertions(+), 50 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/IndexSettings.java b/server/src/main/java/org/opensearch/index/IndexSettings.java index 38a3817fd6f9c..b735a9e4e3ea0 100644 --- a/server/src/main/java/org/opensearch/index/IndexSettings.java +++ b/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); diff --git a/server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java b/server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java index 6d53787c989a3..c20ddcee6da62 100644 --- a/server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java +++ b/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 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,7 +109,6 @@ 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 < MSN) - * @param minimumSupportedVersion for extended backward compatibility use-cases, the minimum {@link Version} this engine is allowed to read */ public ReadOnlyEngine( EngineConfig config, @@ -139,12 +116,12 @@ public ReadOnlyEngine( TranslogStats translogStats, boolean obtainLock, Function 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 diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index 29065936f4e15..9f14ca7d013b2 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesService.java +++ b/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) { diff --git a/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java b/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java index 872b027e7a0d2..81a364c418c95 100644 --- a/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java +++ b/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()); } } } From 99b81ae1a56805bca1c4c3b9d3bbe7517e5a108c Mon Sep 17 00:00:00 2001 From: Kartik Ganesh Date: Wed, 4 Jan 2023 11:20:38 -0800 Subject: [PATCH 10/10] Fix IndexSettingsTests Updated the asserts in IndexSettingsTests to account for the new defaulting behavior. Signed-off-by: Kartik Ganesh --- .../test/java/org/opensearch/index/IndexSettingsTests.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java index ebfec00b97d2d..e0f1066587e5b 100644 --- a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java +++ b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java @@ -974,10 +974,10 @@ public void testExtendedCompatibilityVersionForNonRemoteSnapshot() { ); IndexSettings settings = new IndexSettings(metadata, Settings.EMPTY); assertFalse(settings.isRemoteSnapshot()); - assertNull(settings.getExtendedCompatibilitySnapshotVersion()); + assertEquals(Version.CURRENT.minimumIndexCompatibilityVersion(), settings.getExtendedCompatibilitySnapshotVersion()); } - public void testExtendedCompatibilityVersionMissingKey() { + public void testExtendedCompatibilityVersionWithoutFeatureFlag() { IndexMetadata metadata = newIndexMeta( "index", Settings.builder() @@ -987,6 +987,6 @@ public void testExtendedCompatibilityVersionMissingKey() { ); IndexSettings settings = new IndexSettings(metadata, Settings.EMPTY); assertTrue(settings.isRemoteSnapshot()); - assertNull(settings.getExtendedCompatibilitySnapshotVersion()); + assertEquals(Version.CURRENT.minimumIndexCompatibilityVersion(), settings.getExtendedCompatibilitySnapshotVersion()); } }