From b58bec4de985570461f3bcb8237d65082de9908f Mon Sep 17 00:00:00 2001 From: David Z <38449481+dzane17@users.noreply.github.com> Date: Tue, 15 Aug 2023 15:19:21 -0700 Subject: [PATCH] Default to mmapfs within hybridfs (#8508) * Default to mmapfs within hybridfs Signed-off-by: David Zane * Add index setting validation func Signed-off-by: David Zane * Reviewer comments Signed-off-by: David Zane * Clean up, mmap.extensions validation Signed-off-by: David Zane * Deprecation flag, build all_ext list Signed-off-by: David Zane * Make nioExtensions unmodifiable Signed-off-by: David Zane --------- Signed-off-by: David Zane --- CHANGELOG.md | 1 + .../common/settings/IndexScopedSettings.java | 1 + .../org/opensearch/index/IndexModule.java | 97 +++++++++++++++- .../index/store/FsDirectoryFactory.java | 29 ++++- .../index/store/FsDirectoryFactoryTests.java | 107 +++++++++++++++++- 5 files changed, 222 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e9fac29329937..c1f2e661ddadb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -117,6 +117,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Bump `aws-actions/configure-aws-credentials` from 1 to 2 ([#9302](https://github.com/opensearch-project/OpenSearch/pull/9302)) ### Changed +- Default to mmapfs within hybridfs ([#8508](https://github.com/opensearch-project/OpenSearch/pull/8508)) - Perform aggregation postCollection in ContextIndexSearcher after searching leaves ([#8303](https://github.com/opensearch-project/OpenSearch/pull/8303)) - Make Span exporter configurable ([#8620](https://github.com/opensearch-project/OpenSearch/issues/8620)) - Perform aggregation postCollection in ContextIndexSearcher after searching leaves ([#8303](https://github.com/opensearch-project/OpenSearch/pull/8303)) 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 be2b5f00bc0ec..f14db4354f196 100644 --- a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java @@ -183,6 +183,7 @@ public final class IndexScopedSettings extends AbstractScopedSettings { IndexModule.INDEX_STORE_TYPE_SETTING, IndexModule.INDEX_STORE_PRE_LOAD_SETTING, IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS, + IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS, IndexModule.INDEX_RECOVERY_TYPE_SETTING, IndexModule.INDEX_QUERY_CACHE_ENABLED_SETTING, FsDirectoryFactory.INDEX_LOCK_FACTOR_SETTING, diff --git a/server/src/main/java/org/opensearch/index/IndexModule.java b/server/src/main/java/org/opensearch/index/IndexModule.java index 8a0d563d51107..ff91fd65b6f4e 100644 --- a/server/src/main/java/org/opensearch/index/IndexModule.java +++ b/server/src/main/java/org/opensearch/index/IndexModule.java @@ -95,6 +95,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.Iterator; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.BiFunction; import java.util.function.BooleanSupplier; @@ -154,14 +155,106 @@ public final class IndexModule { Property.NodeScope ); - /** Which lucene file extensions to load with the mmap directory when using hybridfs store. + /** Which lucene file extensions to load with the mmap directory when using hybridfs store. This settings is ignored if {@link #INDEX_STORE_HYBRID_NIO_EXTENSIONS} is set. * This is an expert setting. - * @see Lucene File Extensions. + * @see Lucene File Extensions. + * + * @deprecated This setting will be removed in OpenSearch 3.x. Use {@link #INDEX_STORE_HYBRID_NIO_EXTENSIONS} instead. */ + @Deprecated public static final Setting> INDEX_STORE_HYBRID_MMAP_EXTENSIONS = Setting.listSetting( "index.store.hybrid.mmap.extensions", List.of("nvd", "dvd", "tim", "tip", "dim", "kdd", "kdi", "cfs", "doc"), Function.identity(), + new Setting.Validator>() { + + @Override + public void validate(final List value) {} + + @Override + public void validate(final List value, final Map, Object> settings) { + if (value.equals(INDEX_STORE_HYBRID_MMAP_EXTENSIONS.getDefault(Settings.EMPTY)) == false) { + final List nioExtensions = (List) settings.get(INDEX_STORE_HYBRID_NIO_EXTENSIONS); + final List defaultNioExtensions = INDEX_STORE_HYBRID_NIO_EXTENSIONS.getDefault(Settings.EMPTY); + if (nioExtensions.equals(defaultNioExtensions) == false) { + throw new IllegalArgumentException( + "Settings " + + INDEX_STORE_HYBRID_NIO_EXTENSIONS.getKey() + + " & " + + INDEX_STORE_HYBRID_MMAP_EXTENSIONS.getKey() + + " cannot both be set. Use " + + INDEX_STORE_HYBRID_NIO_EXTENSIONS.getKey() + + " only." + ); + } + } + } + + @Override + public Iterator> settings() { + return List.>of(INDEX_STORE_HYBRID_NIO_EXTENSIONS).iterator(); + } + }, + Property.IndexScope, + Property.NodeScope, + Property.Deprecated + ); + + /** Which lucene file extensions to load with nio. All others will default to mmap. Takes precedence over {@link #INDEX_STORE_HYBRID_MMAP_EXTENSIONS}. + * This is an expert setting. + * @see Lucene File Extensions. + */ + public static final Setting> INDEX_STORE_HYBRID_NIO_EXTENSIONS = Setting.listSetting( + "index.store.hybrid.nio.extensions", + List.of( + "segments_N", + "write.lock", + "si", + "cfe", + "fnm", + "fdx", + "fdt", + "pos", + "pay", + "nvm", + "dvm", + "tvx", + "tvd", + "liv", + "dii", + "vec", + "vem" + ), + Function.identity(), + new Setting.Validator>() { + + @Override + public void validate(final List value) {} + + @Override + public void validate(final List value, final Map, Object> settings) { + if (value.equals(INDEX_STORE_HYBRID_NIO_EXTENSIONS.getDefault(Settings.EMPTY)) == false) { + final List mmapExtensions = (List) settings.get(INDEX_STORE_HYBRID_MMAP_EXTENSIONS); + final List defaultMmapExtensions = INDEX_STORE_HYBRID_MMAP_EXTENSIONS.getDefault(Settings.EMPTY); + if (mmapExtensions.equals(defaultMmapExtensions) == false) { + throw new IllegalArgumentException( + "Settings " + + INDEX_STORE_HYBRID_NIO_EXTENSIONS.getKey() + + " & " + + INDEX_STORE_HYBRID_MMAP_EXTENSIONS.getKey() + + " cannot both be set. Use " + + INDEX_STORE_HYBRID_NIO_EXTENSIONS.getKey() + + " only." + ); + } + } + } + + @Override + public Iterator> settings() { + return List.>of(INDEX_STORE_HYBRID_MMAP_EXTENSIONS).iterator(); + } + }, Property.IndexScope, Property.NodeScope ); diff --git a/server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java b/server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java index 3b5b4040954c9..9b5bc8f94ce35 100644 --- a/server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java +++ b/server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java @@ -44,6 +44,7 @@ import org.apache.lucene.store.NativeFSLockFactory; import org.apache.lucene.store.SimpleFSLockFactory; import org.opensearch.common.settings.Setting; +import org.opensearch.common.settings.Settings; import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.util.io.IOUtils; import org.opensearch.index.IndexModule; @@ -56,6 +57,8 @@ import java.nio.file.Path; import java.util.HashSet; import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; /** * Factory for a filesystem directory @@ -97,10 +100,24 @@ protected Directory newFSDirectory(Path location, LockFactory lockFactory, Index case HYBRIDFS: // Use Lucene defaults final FSDirectory primaryDirectory = FSDirectory.open(location, lockFactory); - final Set mmapExtensions = new HashSet<>(indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS)); + final Set nioExtensions; + final Set mmapExtensions = Set.copyOf(indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS)); + if (mmapExtensions.equals( + new HashSet(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS.getDefault(Settings.EMPTY)) + ) == false) { + // If the mmap extension setting was defined, then compute nio extensions by subtracting out the + // mmap extensions from the set of all extensions. + nioExtensions = Stream.concat( + IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS.getDefault(Settings.EMPTY).stream(), + IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS.getDefault(Settings.EMPTY).stream() + ).filter(e -> mmapExtensions.contains(e) == false).collect(Collectors.toUnmodifiableSet()); + } else { + // Otherwise, get the list of nio extensions from the nio setting + nioExtensions = Set.copyOf(indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS)); + } if (primaryDirectory instanceof MMapDirectory) { MMapDirectory mMapDirectory = (MMapDirectory) primaryDirectory; - return new HybridDirectory(lockFactory, setPreload(mMapDirectory, lockFactory, preLoadExtensions), mmapExtensions); + return new HybridDirectory(lockFactory, setPreload(mMapDirectory, lockFactory, preLoadExtensions), nioExtensions); } else { return primaryDirectory; } @@ -143,12 +160,12 @@ public static boolean isHybridFs(Directory directory) { */ static final class HybridDirectory extends NIOFSDirectory { private final MMapDirectory delegate; - private final Set mmapExtensions; + private final Set nioExtensions; - HybridDirectory(LockFactory lockFactory, MMapDirectory delegate, Set mmapExtensions) throws IOException { + HybridDirectory(LockFactory lockFactory, MMapDirectory delegate, Set nioExtensions) throws IOException { super(delegate.getDirectory(), lockFactory); this.delegate = delegate; - this.mmapExtensions = mmapExtensions; + this.nioExtensions = nioExtensions; } @Override @@ -169,7 +186,7 @@ public IndexInput openInput(String name, IOContext context) throws IOException { boolean useDelegate(String name) { final String extension = FileSwitchDirectory.getExtension(name); - return mmapExtensions.contains(extension); + return nioExtensions.contains(extension) == false; } @Override diff --git a/server/src/test/java/org/opensearch/index/store/FsDirectoryFactoryTests.java b/server/src/test/java/org/opensearch/index/store/FsDirectoryFactoryTests.java index 56d67820797a2..bc9ee9d3d4fb6 100644 --- a/server/src/test/java/org/opensearch/index/store/FsDirectoryFactoryTests.java +++ b/server/src/test/java/org/opensearch/index/store/FsDirectoryFactoryTests.java @@ -72,7 +72,8 @@ public void testPreload() throws IOException { try (Directory directory = newDirectory(build)) { assertTrue(FsDirectoryFactory.isHybridFs(directory)); FsDirectoryFactory.HybridDirectory hybridDirectory = (FsDirectoryFactory.HybridDirectory) directory; - // test default hybrid mmap extensions + // test default hybrid extensions + // true->mmap, false->nio assertTrue(hybridDirectory.useDelegate("foo.nvd")); assertTrue(hybridDirectory.useDelegate("foo.dvd")); assertTrue(hybridDirectory.useDelegate("foo.tim")); @@ -82,6 +83,7 @@ public void testPreload() throws IOException { assertTrue(hybridDirectory.useDelegate("foo.kdi")); assertTrue(hybridDirectory.useDelegate("foo.cfs")); assertTrue(hybridDirectory.useDelegate("foo.doc")); + assertTrue(hybridDirectory.useDelegate("foo.new")); assertFalse(hybridDirectory.useDelegate("foo.pos")); assertFalse(hybridDirectory.useDelegate("foo.pay")); MMapDirectory delegate = hybridDirectory.getDelegate(); @@ -94,23 +96,25 @@ public void testPreload() throws IOException { build = Settings.builder() .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.HYBRIDFS.name().toLowerCase(Locale.ROOT)) .putList(IndexModule.INDEX_STORE_PRE_LOAD_SETTING.getKey(), "nvd", "dvd", "cfs") - .putList(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS.getKey(), "nvd", "dvd", "tim", "pos", "pay") + .putList(IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS.getKey(), "tip", "dim", "kdd", "kdi", "cfs", "doc") .build(); try (Directory directory = newDirectory(build)) { assertTrue(FsDirectoryFactory.isHybridFs(directory)); FsDirectoryFactory.HybridDirectory hybridDirectory = (FsDirectoryFactory.HybridDirectory) directory; - // test custom hybrid mmap extensions + // test custom hybrid nio extensions + // true->mmap, false->nio assertTrue(hybridDirectory.useDelegate("foo.nvd")); assertTrue(hybridDirectory.useDelegate("foo.dvd")); assertTrue(hybridDirectory.useDelegate("foo.tim")); + assertTrue(hybridDirectory.useDelegate("foo.pos")); + assertTrue(hybridDirectory.useDelegate("foo.pay")); + assertTrue(hybridDirectory.useDelegate("foo.new")); assertFalse(hybridDirectory.useDelegate("foo.tip")); assertFalse(hybridDirectory.useDelegate("foo.dim")); assertFalse(hybridDirectory.useDelegate("foo.kdd")); assertFalse(hybridDirectory.useDelegate("foo.kdi")); assertFalse(hybridDirectory.useDelegate("foo.cfs")); assertFalse(hybridDirectory.useDelegate("foo.doc")); - assertTrue(hybridDirectory.useDelegate("foo.pos")); - assertTrue(hybridDirectory.useDelegate("foo.pay")); MMapDirectory delegate = hybridDirectory.getDelegate(); assertThat(delegate, Matchers.instanceOf(FsDirectoryFactory.PreLoadMMapDirectory.class)); FsDirectoryFactory.PreLoadMMapDirectory preLoadMMapDirectory = (FsDirectoryFactory.PreLoadMMapDirectory) delegate; @@ -119,6 +123,99 @@ public void testPreload() throws IOException { assertTrue(preLoadMMapDirectory.useDelegate("foo.cfs")); assertTrue(preLoadMMapDirectory.useDelegate("foo.nvd")); } + build = Settings.builder() + .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.HYBRIDFS.name().toLowerCase(Locale.ROOT)) + .putList(IndexModule.INDEX_STORE_PRE_LOAD_SETTING.getKey(), "nvd", "dvd", "cfs") + .putList(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS.getKey(), "nvd", "dvd", "tim", "pos") + .build(); + try (Directory directory = newDirectory(build)) { + assertTrue(FsDirectoryFactory.isHybridFs(directory)); + FsDirectoryFactory.HybridDirectory hybridDirectory = (FsDirectoryFactory.HybridDirectory) directory; + // test custom hybrid mmap extensions + // true->mmap, false->nio + assertTrue(hybridDirectory.useDelegate("foo.nvd")); + assertTrue(hybridDirectory.useDelegate("foo.dvd")); + assertTrue(hybridDirectory.useDelegate("foo.tim")); + assertTrue(hybridDirectory.useDelegate("foo.pos")); + assertTrue(hybridDirectory.useDelegate("foo.new")); + assertFalse(hybridDirectory.useDelegate("foo.pay")); + assertFalse(hybridDirectory.useDelegate("foo.tip")); + assertFalse(hybridDirectory.useDelegate("foo.dim")); + assertFalse(hybridDirectory.useDelegate("foo.kdd")); + assertFalse(hybridDirectory.useDelegate("foo.kdi")); + assertFalse(hybridDirectory.useDelegate("foo.cfs")); + assertFalse(hybridDirectory.useDelegate("foo.doc")); + MMapDirectory delegate = hybridDirectory.getDelegate(); + assertThat(delegate, Matchers.instanceOf(FsDirectoryFactory.PreLoadMMapDirectory.class)); + assertWarnings( + "[index.store.hybrid.mmap.extensions] setting was deprecated in OpenSearch and will be removed in a future release!" + + " See the breaking changes documentation for the next major version." + ); + } + build = Settings.builder() + .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.HYBRIDFS.name().toLowerCase(Locale.ROOT)) + .putList(IndexModule.INDEX_STORE_PRE_LOAD_SETTING.getKey(), "nvd", "dvd", "cfs") + .putList(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS.getKey(), "nvd", "dvd", "tim", "pos") + .putList(IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS.getKey(), "nvd", "dvd", "tim", "pos") + .build(); + try { + newDirectory(build); + } catch (final Exception e) { + assertEquals( + "Settings index.store.hybrid.nio.extensions & index.store.hybrid.mmap.extensions cannot both be set. Use index.store.hybrid.nio.extensions only.", + e.getMessage() + ); + } + build = Settings.builder() + .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.HYBRIDFS.name().toLowerCase(Locale.ROOT)) + .putList(IndexModule.INDEX_STORE_PRE_LOAD_SETTING.getKey(), "nvd", "dvd", "cfs") + .putList(IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS.getKey(), "nvd", "dvd", "tim", "pos") + .putList(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS.getKey(), "nvd", "dvd", "tim", "pos") + .build(); + try { + newDirectory(build); + } catch (final Exception e) { + assertEquals( + "Settings index.store.hybrid.nio.extensions & index.store.hybrid.mmap.extensions cannot both be set. Use index.store.hybrid.nio.extensions only.", + e.getMessage() + ); + } + build = Settings.builder() + .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.HYBRIDFS.name().toLowerCase(Locale.ROOT)) + .putList(IndexModule.INDEX_STORE_PRE_LOAD_SETTING.getKey(), "nvd", "dvd", "cfs") + .putList(IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS.getKey()) + .build(); + try (Directory directory = newDirectory(build)) { + assertTrue(FsDirectoryFactory.isHybridFs(directory)); + FsDirectoryFactory.HybridDirectory hybridDirectory = (FsDirectoryFactory.HybridDirectory) directory; + // test custom hybrid mmap extensions + // true->mmap, false->nio + assertTrue(hybridDirectory.useDelegate("foo.new")); + assertTrue(hybridDirectory.useDelegate("foo.nvd")); + assertTrue(hybridDirectory.useDelegate("foo.dvd")); + assertTrue(hybridDirectory.useDelegate("foo.cfs")); + assertTrue(hybridDirectory.useDelegate("foo.doc")); + MMapDirectory delegate = hybridDirectory.getDelegate(); + assertThat(delegate, Matchers.instanceOf(FsDirectoryFactory.PreLoadMMapDirectory.class)); + } + build = Settings.builder() + .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.HYBRIDFS.name().toLowerCase(Locale.ROOT)) + .putList(IndexModule.INDEX_STORE_PRE_LOAD_SETTING.getKey(), "nvd", "dvd", "cfs") + .putList(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS.getKey()) + .build(); + try (Directory directory = newDirectory(build)) { + assertTrue(FsDirectoryFactory.isHybridFs(directory)); + FsDirectoryFactory.HybridDirectory hybridDirectory = (FsDirectoryFactory.HybridDirectory) directory; + // test custom hybrid mmap extensions + // true->mmap, false->nio + assertTrue(hybridDirectory.useDelegate("foo.new")); + assertFalse(hybridDirectory.useDelegate("foo.nvd")); + assertFalse(hybridDirectory.useDelegate("foo.dvd")); + assertFalse(hybridDirectory.useDelegate("foo.cfs")); + assertFalse(hybridDirectory.useDelegate("foo.doc")); + MMapDirectory delegate = hybridDirectory.getDelegate(); + assertThat(delegate, Matchers.instanceOf(FsDirectoryFactory.PreLoadMMapDirectory.class)); + } } private Directory newDirectory(Settings settings) throws IOException {