From 25becfbc8b1d8f657f1b3b90d5195579712e5679 Mon Sep 17 00:00:00 2001 From: David Zane Date: Mon, 17 Jul 2023 11:53:40 -0700 Subject: [PATCH 1/6] Default to mmapfs within hybridfs Signed-off-by: David Zane --- CHANGELOG.md | 1 + .../common/settings/IndexScopedSettings.java | 1 + .../org/opensearch/index/IndexModule.java | 19 ++++++- .../index/store/FsDirectoryFactory.java | 55 +++++++++++++++++-- .../index/store/FsDirectoryFactoryTests.java | 45 ++++++++++++++- 5 files changed, 110 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f231d1db8036b..69b825afef2ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -123,6 +123,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)) - Change InternalSignificantTerms to sum shard-level superset counts only in final reduce ([#8735](https://github.com/opensearch-project/OpenSearch/pull/8735)) 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..db39d768a1f89 100644 --- a/server/src/main/java/org/opensearch/index/IndexModule.java +++ b/server/src/main/java/org/opensearch/index/IndexModule.java @@ -154,10 +154,13 @@ 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 4.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"), @@ -166,6 +169,18 @@ public final class IndexModule { Property.NodeScope ); + /** 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", + Collections.emptyList(), + Function.identity(), + Property.IndexScope, + Property.NodeScope + ); + public static final String SIMILARITY_SETTINGS_PREFIX = "index.similarity"; // whether to use the query cache 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..88c0433b87c29 100644 --- a/server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java +++ b/server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java @@ -55,6 +55,8 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.HashSet; +import java.util.ArrayList; +import java.util.List; import java.util.Set; /** @@ -97,10 +99,22 @@ 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)); + Set nioExtensions = new HashSet<>(indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS)); + final List mmapExtensions = new ArrayList<>(indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS)); + if (nioExtensions.isEmpty()) { + List allExtensions = new ArrayList<>(INDEX_STORE_HYBRID_ALL_EXTENSIONS); + allExtensions.removeAll(mmapExtensions); + nioExtensions = new HashSet<>(allExtensions); + } else { + if (!mmapExtensions.equals(List.of("nvd", "dvd", "tim", "tip", "dim", "kdd", "kdi", "cfs", "doc"))) { + throw new IllegalArgumentException( + "INDEX_STORE_HYBRID_MMAP_EXTENSIONS and INDEX_STORE_HYBRID_NIO_EXTENSIONS are both defined. Use INDEX_STORE_HYBRID_NIO_EXTENSIONS only" + ); + } + } 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 +157,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 +183,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); } @Override @@ -232,4 +246,33 @@ MMapDirectory getDelegate() { return delegate; } } + + private static List INDEX_STORE_HYBRID_ALL_EXTENSIONS = List.of( + "nvd", // mmap defaults start + "dvd", + "tim", + "tip", + "dim", + "kdd", + "kdi", + "cfs", + "doc", // mmap defaults end + "segments_N", // nio defaults start + "write.lock", + "si", + "cfe", + "fnm", + "fdx", + "fdt", + "pos", + "pay", + "nvm", + "dvm", + "tvx", + "tvd", + "liv", + "dii", + "vec", + "vem" // nio defaults end + ); } 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..2eecbad7956eb 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,7 @@ 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 assertTrue(hybridDirectory.useDelegate("foo.nvd")); assertTrue(hybridDirectory.useDelegate("foo.dvd")); assertTrue(hybridDirectory.useDelegate("foo.tim")); @@ -82,6 +82,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,12 +95,12 @@ 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 assertTrue(hybridDirectory.useDelegate("foo.nvd")); assertTrue(hybridDirectory.useDelegate("foo.dvd")); assertTrue(hybridDirectory.useDelegate("foo.tim")); @@ -119,6 +120,44 @@ 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 + 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)); + } + 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( + e.getMessage(), + "INDEX_STORE_HYBRID_MMAP_EXTENSIONS and INDEX_STORE_HYBRID_NIO_EXTENSIONS are both defined. Use INDEX_STORE_HYBRID_NIO_EXTENSIONS only" + ); + } } private Directory newDirectory(Settings settings) throws IOException { From db3a168255410f484a3ab0051e4b3ecbb2894cd1 Mon Sep 17 00:00:00 2001 From: David Zane Date: Tue, 25 Jul 2023 16:40:52 -0700 Subject: [PATCH 2/6] Add index setting validation func Signed-off-by: David Zane --- .../org/opensearch/index/IndexModule.java | 30 +++++++++++++++++++ .../index/store/FsDirectoryFactory.java | 6 ---- .../index/store/FsDirectoryFactoryTests.java | 4 +-- 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/IndexModule.java b/server/src/main/java/org/opensearch/index/IndexModule.java index db39d768a1f89..abf8aa66803e0 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; @@ -177,6 +178,35 @@ public final class IndexModule { "index.store.hybrid.nio.extensions", Collections.emptyList(), 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.isEmpty()) { + final List mmapExtensions = (List) settings.get(INDEX_STORE_HYBRID_MMAP_EXTENSIONS); + if (!mmapExtensions.equals(List.of("nvd", "dvd", "tim", "tip", "dim", "kdd", "kdi", "cfs", "doc"))) { + 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() { + final List> settings = Collections.singletonList(INDEX_STORE_HYBRID_MMAP_EXTENSIONS); + return settings.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 88c0433b87c29..a9fe0265743bd 100644 --- a/server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java +++ b/server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java @@ -105,12 +105,6 @@ protected Directory newFSDirectory(Path location, LockFactory lockFactory, Index List allExtensions = new ArrayList<>(INDEX_STORE_HYBRID_ALL_EXTENSIONS); allExtensions.removeAll(mmapExtensions); nioExtensions = new HashSet<>(allExtensions); - } else { - if (!mmapExtensions.equals(List.of("nvd", "dvd", "tim", "tip", "dim", "kdd", "kdi", "cfs", "doc"))) { - throw new IllegalArgumentException( - "INDEX_STORE_HYBRID_MMAP_EXTENSIONS and INDEX_STORE_HYBRID_NIO_EXTENSIONS are both defined. Use INDEX_STORE_HYBRID_NIO_EXTENSIONS only" - ); - } } if (primaryDirectory instanceof MMapDirectory) { MMapDirectory mMapDirectory = (MMapDirectory) primaryDirectory; 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 2eecbad7956eb..4000ee53e96aa 100644 --- a/server/src/test/java/org/opensearch/index/store/FsDirectoryFactoryTests.java +++ b/server/src/test/java/org/opensearch/index/store/FsDirectoryFactoryTests.java @@ -154,8 +154,8 @@ public void testPreload() throws IOException { newDirectory(build); } catch (final Exception e) { assertEquals( - e.getMessage(), - "INDEX_STORE_HYBRID_MMAP_EXTENSIONS and INDEX_STORE_HYBRID_NIO_EXTENSIONS are both defined. Use INDEX_STORE_HYBRID_NIO_EXTENSIONS only" + "Settings index.store.hybrid.nio.extensions & index.store.hybrid.mmap.extensions cannot both be set. Use index.store.hybrid.nio.extensions only.", + e.getMessage() ); } } From bee285bea94373a37af8efb0bccc26729eb0fca5 Mon Sep 17 00:00:00 2001 From: David Zane Date: Wed, 26 Jul 2023 10:31:44 -0700 Subject: [PATCH 3/6] Reviewer comments Signed-off-by: David Zane --- server/src/main/java/org/opensearch/index/IndexModule.java | 6 +++--- .../org/opensearch/index/store/FsDirectoryFactory.java | 7 ++----- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/IndexModule.java b/server/src/main/java/org/opensearch/index/IndexModule.java index abf8aa66803e0..5927da60bc0d5 100644 --- a/server/src/main/java/org/opensearch/index/IndexModule.java +++ b/server/src/main/java/org/opensearch/index/IndexModule.java @@ -187,7 +187,8 @@ public void validate(final List value) {} public void validate(final List value, final Map, Object> settings) { if (!value.isEmpty()) { final List mmapExtensions = (List) settings.get(INDEX_STORE_HYBRID_MMAP_EXTENSIONS); - if (!mmapExtensions.equals(List.of("nvd", "dvd", "tim", "tip", "dim", "kdd", "kdi", "cfs", "doc"))) { + final List defaultMmapExtensions = INDEX_STORE_HYBRID_MMAP_EXTENSIONS.getDefault(Settings.EMPTY); + if (!mmapExtensions.equals(defaultMmapExtensions)) { throw new IllegalArgumentException( "Settings " + INDEX_STORE_HYBRID_NIO_EXTENSIONS.getKey() @@ -203,8 +204,7 @@ public void validate(final List value, final Map, Object> set @Override public Iterator> settings() { - final List> settings = Collections.singletonList(INDEX_STORE_HYBRID_MMAP_EXTENSIONS); - return settings.iterator(); + return List.>of(INDEX_STORE_HYBRID_MMAP_EXTENSIONS).iterator(); } }, Property.IndexScope, 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 a9fe0265743bd..e80c0af24bfea 100644 --- a/server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java +++ b/server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java @@ -55,7 +55,6 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.HashSet; -import java.util.ArrayList; import java.util.List; import java.util.Set; @@ -100,11 +99,9 @@ protected Directory newFSDirectory(Path location, LockFactory lockFactory, Index // Use Lucene defaults final FSDirectory primaryDirectory = FSDirectory.open(location, lockFactory); Set nioExtensions = new HashSet<>(indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS)); - final List mmapExtensions = new ArrayList<>(indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS)); if (nioExtensions.isEmpty()) { - List allExtensions = new ArrayList<>(INDEX_STORE_HYBRID_ALL_EXTENSIONS); - allExtensions.removeAll(mmapExtensions); - nioExtensions = new HashSet<>(allExtensions); + nioExtensions.addAll(INDEX_STORE_HYBRID_ALL_EXTENSIONS); + nioExtensions.removeAll(indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS)); } if (primaryDirectory instanceof MMapDirectory) { MMapDirectory mMapDirectory = (MMapDirectory) primaryDirectory; From e49e3c54918bfc4a4ec5ce2b0765b3c6d253cc31 Mon Sep 17 00:00:00 2001 From: David Zane Date: Thu, 3 Aug 2023 15:25:05 -0700 Subject: [PATCH 4/6] Clean up, mmap.extensions validation Signed-off-by: David Zane --- .../org/opensearch/index/IndexModule.java | 53 ++++++++++++++++- .../index/store/FsDirectoryFactory.java | 8 ++- .../index/store/FsDirectoryFactoryTests.java | 58 ++++++++++++++++++- 3 files changed, 111 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/IndexModule.java b/server/src/main/java/org/opensearch/index/IndexModule.java index 5927da60bc0d5..fe19a9ef19cd3 100644 --- a/server/src/main/java/org/opensearch/index/IndexModule.java +++ b/server/src/main/java/org/opensearch/index/IndexModule.java @@ -166,6 +166,35 @@ public final class IndexModule { "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 ); @@ -176,7 +205,25 @@ public final class IndexModule { */ public static final Setting> INDEX_STORE_HYBRID_NIO_EXTENSIONS = Setting.listSetting( "index.store.hybrid.nio.extensions", - Collections.emptyList(), + 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>() { @@ -185,10 +232,10 @@ public void validate(final List value) {} @Override public void validate(final List value, final Map, Object> settings) { - if (!value.isEmpty()) { + 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)) { + if (mmapExtensions.equals(defaultMmapExtensions) == false) { throw new IllegalArgumentException( "Settings " + INDEX_STORE_HYBRID_NIO_EXTENSIONS.getKey() 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 e80c0af24bfea..ddb1bb71c55b5 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; @@ -99,8 +100,9 @@ protected Directory newFSDirectory(Path location, LockFactory lockFactory, Index // Use Lucene defaults final FSDirectory primaryDirectory = FSDirectory.open(location, lockFactory); Set nioExtensions = new HashSet<>(indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS)); - if (nioExtensions.isEmpty()) { - nioExtensions.addAll(INDEX_STORE_HYBRID_ALL_EXTENSIONS); + if (indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS) + .equals(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS.getDefault(Settings.EMPTY)) == false) { + nioExtensions = new HashSet<>(INDEX_STORE_HYBRID_ALL_EXTENSIONS); nioExtensions.removeAll(indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS)); } if (primaryDirectory instanceof MMapDirectory) { @@ -174,7 +176,7 @@ public IndexInput openInput(String name, IOContext context) throws IOException { boolean useDelegate(String name) { final String extension = FileSwitchDirectory.getExtension(name); - return !nioExtensions.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 4000ee53e96aa..c40a7b460904a 100644 --- a/server/src/test/java/org/opensearch/index/store/FsDirectoryFactoryTests.java +++ b/server/src/test/java/org/opensearch/index/store/FsDirectoryFactoryTests.java @@ -73,6 +73,7 @@ public void testPreload() throws IOException { assertTrue(FsDirectoryFactory.isHybridFs(directory)); FsDirectoryFactory.HybridDirectory hybridDirectory = (FsDirectoryFactory.HybridDirectory) directory; // test default hybrid extensions + // true->mmap, false->nio assertTrue(hybridDirectory.useDelegate("foo.nvd")); assertTrue(hybridDirectory.useDelegate("foo.dvd")); assertTrue(hybridDirectory.useDelegate("foo.tim")); @@ -101,17 +102,19 @@ public void testPreload() throws IOException { assertTrue(FsDirectoryFactory.isHybridFs(directory)); FsDirectoryFactory.HybridDirectory hybridDirectory = (FsDirectoryFactory.HybridDirectory) directory; // 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; @@ -129,6 +132,7 @@ public void testPreload() throws IOException { 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")); @@ -158,6 +162,56 @@ public void testPreload() throws IOException { 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 { From 0794f6a401cc77383d305597973c52a1d88e5029 Mon Sep 17 00:00:00 2001 From: David Zane Date: Tue, 8 Aug 2023 11:29:01 -0700 Subject: [PATCH 5/6] Deprecation flag, build all_ext list Signed-off-by: David Zane --- .../org/opensearch/index/IndexModule.java | 5 ++- .../index/store/FsDirectoryFactory.java | 37 +++---------------- .../index/store/FsDirectoryFactoryTests.java | 4 ++ 3 files changed, 13 insertions(+), 33 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/IndexModule.java b/server/src/main/java/org/opensearch/index/IndexModule.java index fe19a9ef19cd3..ff91fd65b6f4e 100644 --- a/server/src/main/java/org/opensearch/index/IndexModule.java +++ b/server/src/main/java/org/opensearch/index/IndexModule.java @@ -159,7 +159,7 @@ public final class IndexModule { * This is an expert setting. * @see Lucene File Extensions. * - * @deprecated This setting will be removed in OpenSearch 4.x. Use {@link #INDEX_STORE_HYBRID_NIO_EXTENSIONS} instead. + * @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( @@ -196,7 +196,8 @@ public Iterator> settings() { } }, Property.IndexScope, - Property.NodeScope + 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}. 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 ddb1bb71c55b5..564c443f78dda 100644 --- a/server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java +++ b/server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java @@ -56,8 +56,9 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.HashSet; -import java.util.List; import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; /** * Factory for a filesystem directory @@ -102,7 +103,10 @@ protected Directory newFSDirectory(Path location, LockFactory lockFactory, Index Set nioExtensions = new HashSet<>(indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS)); if (indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS) .equals(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS.getDefault(Settings.EMPTY)) == false) { - nioExtensions = new HashSet<>(INDEX_STORE_HYBRID_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() + ).collect(Collectors.toSet()); nioExtensions.removeAll(indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS)); } if (primaryDirectory instanceof MMapDirectory) { @@ -239,33 +243,4 @@ MMapDirectory getDelegate() { return delegate; } } - - private static List INDEX_STORE_HYBRID_ALL_EXTENSIONS = List.of( - "nvd", // mmap defaults start - "dvd", - "tim", - "tip", - "dim", - "kdd", - "kdi", - "cfs", - "doc", // mmap defaults end - "segments_N", // nio defaults start - "write.lock", - "si", - "cfe", - "fnm", - "fdx", - "fdt", - "pos", - "pay", - "nvm", - "dvm", - "tvx", - "tvd", - "liv", - "dii", - "vec", - "vem" // nio defaults end - ); } 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 c40a7b460904a..bc9ee9d3d4fb6 100644 --- a/server/src/test/java/org/opensearch/index/store/FsDirectoryFactoryTests.java +++ b/server/src/test/java/org/opensearch/index/store/FsDirectoryFactoryTests.java @@ -147,6 +147,10 @@ public void testPreload() throws IOException { 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)) From b8e1e09a2291f70d531ece0698fe245cf3f4f54d Mon Sep 17 00:00:00 2001 From: David Zane Date: Tue, 15 Aug 2023 14:39:26 -0500 Subject: [PATCH 6/6] Make nioExtensions unmodifiable Signed-off-by: David Zane --- .../index/store/FsDirectoryFactory.java | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) 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 564c443f78dda..9b5bc8f94ce35 100644 --- a/server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java +++ b/server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java @@ -100,14 +100,20 @@ protected Directory newFSDirectory(Path location, LockFactory lockFactory, Index case HYBRIDFS: // Use Lucene defaults final FSDirectory primaryDirectory = FSDirectory.open(location, lockFactory); - Set nioExtensions = new HashSet<>(indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS)); - if (indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS) - .equals(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS.getDefault(Settings.EMPTY)) == false) { + 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() - ).collect(Collectors.toSet()); - nioExtensions.removeAll(indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS)); + ).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;