Remove mmap.extensions setting (opensearch-project#9392)

* Remove mmap.extensions setting in favor of nio.extensions Signed-off-by: David Zane <[email protected]> * Update CHANGELOG-3.0.md Co-authored-by: Andriy Redko <[email protected]> Signed-off-by: David Zane <[email protected]> --------- Signed-off-by: David Zane <[email protected]> Signed-off-by: David Zane <[email protected]> Co-authored-by: Andriy Redko <[email protected]>
wdongyu · Aug 22, 2024 · e00f5d4 · e00f5d4
1 parent 72b3be1
commit e00f5d4
Show file tree

Hide file tree

Showing 5 changed files with 5 additions and 172 deletions.
diff --git a/CHANGELOG-3.0.md b/CHANGELOG-3.0.md
@@ -43,6 +43,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Remove LegacyESVersion.V_7_10_ Constants ([#5018](https://github.com/opensearch-project/OpenSearch/pull/5018))
 - Remove Version.V_1_ Constants ([#5021](https://github.com/opensearch-project/OpenSearch/pull/5021))
 - Remove custom Map, List and Set collection classes ([#6871](https://github.com/opensearch-project/OpenSearch/pull/6871))
+- Remove `index.store.hybrid.mmap.extensions` setting in favor of `index.store.hybrid.nio.extensions` setting ([#9392](https://github.com/opensearch-project/OpenSearch/pull/9392))
 
 ### Fixed
 - Fix 'org.apache.hc.core5.http.ParseException: Invalid protocol version' under JDK 16+ ([#4827](https://github.com/opensearch-project/OpenSearch/pull/4827))

diff --git a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java
@@ -191,7 +191,6 @@ public final class IndexScopedSettings extends AbstractScopedSettings {
                 BitsetFilterCache.INDEX_LOAD_RANDOM_ACCESS_FILTERS_EAGERLY_SETTING,
                 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,

diff --git a/server/src/main/java/org/opensearch/index/IndexModule.java b/server/src/main/java/org/opensearch/index/IndexModule.java
@@ -97,7 +97,6 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -183,52 +182,7 @@ public final class IndexModule {
         Property.PrivateIndex
     );
 
-    /** 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 <a href="https://lucene.apache.org/core/9_9_0/core/org/apache/lucene/codecs/lucene99/package-summary.html#file-names">Lucene File Extensions</a>.
-     *
-     * @deprecated This setting will be removed in OpenSearch 3.x. Use {@link #INDEX_STORE_HYBRID_NIO_EXTENSIONS} instead.
-     */
-    @Deprecated
-    public static final Setting<List<String>> 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<List<String>>() {
-
-            @Override
-            public void validate(final List<String> value) {}
-
-            @Override
-            public void validate(final List<String> value, final Map<Setting<?>, Object> settings) {
-                if (value.equals(INDEX_STORE_HYBRID_MMAP_EXTENSIONS.getDefault(Settings.EMPTY)) == false) {
-                    final List<String> nioExtensions = (List<String>) settings.get(INDEX_STORE_HYBRID_NIO_EXTENSIONS);
-                    final List<String> 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<Setting<?>> settings() {
-                return List.<Setting<?>>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}.
+    /** Which lucene file extensions to load with nio. All others will default to mmap.
      *  This is an expert setting.
      *  @see <a href="https://lucene.apache.org/core/9_9_0/core/org/apache/lucene/codecs/lucene99/package-summary.html#file-names">Lucene File Extensions</a>.
      */
@@ -253,35 +207,6 @@ public Iterator<Setting<?>> settings() {
             "vem"
         ),
         Function.identity(),
-        new Setting.Validator<List<String>>() {
-
-            @Override
-            public void validate(final List<String> value) {}
-
-            @Override
-            public void validate(final List<String> value, final Map<Setting<?>, Object> settings) {
-                if (value.equals(INDEX_STORE_HYBRID_NIO_EXTENSIONS.getDefault(Settings.EMPTY)) == false) {
-                    final List<String> mmapExtensions = (List<String>) settings.get(INDEX_STORE_HYBRID_MMAP_EXTENSIONS);
-                    final List<String> 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<Setting<?>> settings() {
-                return List.<Setting<?>>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
@@ -45,7 +45,6 @@
 import org.apache.lucene.store.SimpleFSLockFactory;
 import org.opensearch.common.settings.Setting;
 import org.opensearch.common.settings.Setting.Property;
-import org.opensearch.common.settings.Settings;
 import org.opensearch.common.util.io.IOUtils;
 import org.opensearch.index.IndexModule;
 import org.opensearch.index.IndexSettings;
@@ -57,8 +56,6 @@
 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
@@ -100,21 +97,7 @@ protected Directory newFSDirectory(Path location, LockFactory lockFactory, Index
             case HYBRIDFS:
                 // Use Lucene defaults
                 final FSDirectory primaryDirectory = FSDirectory.open(location, lockFactory);
-                final Set<String> nioExtensions;
-                final Set<String> 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));
-                }
+                final Set<String> nioExtensions = new HashSet<>(indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS));
                 if (primaryDirectory instanceof MMapDirectory) {
                     MMapDirectory mMapDirectory = (MMapDirectory) primaryDirectory;
                     return new HybridDirectory(lockFactory, setPreload(mMapDirectory, lockFactory, preLoadExtensions), nioExtensions);

diff --git a/server/src/test/java/org/opensearch/index/store/FsDirectoryFactoryTests.java b/server/src/test/java/org/opensearch/index/store/FsDirectoryFactoryTests.java
@@ -96,7 +96,7 @@ 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_NIO_EXTENSIONS.getKey(), "tip", "dim", "kdd", "kdi", "cfs", "doc")
+            .putList(IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS.getKey(), "tip", "dim", "kdd", "kdi", "cfs", "doc", "new")
             .build();
         try (Directory directory = newDirectory(build)) {
             assertTrue(FsDirectoryFactory.isHybridFs(directory));
@@ -108,7 +108,7 @@ public void testPreload() throws IOException {
             assertTrue(hybridDirectory.useDelegate("foo.tim"));
             assertTrue(hybridDirectory.useDelegate("foo.pos"));
             assertTrue(hybridDirectory.useDelegate("foo.pay"));
-            assertTrue(hybridDirectory.useDelegate("foo.new"));
+            assertFalse(hybridDirectory.useDelegate("foo.new"));
             assertFalse(hybridDirectory.useDelegate("foo.tip"));
             assertFalse(hybridDirectory.useDelegate("foo.dim"));
             assertFalse(hybridDirectory.useDelegate("foo.kdd"));
@@ -123,63 +123,6 @@ 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")
@@ -198,24 +141,6 @@ public void testPreload() throws IOException {
             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 {