From ace40a2a6f11cec5290f85f06b0c854640d1af31 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Tue, 31 Mar 2026 22:02:53 +0000 Subject: [PATCH] Update MMapDirectory to use ADVISE_BY_CONTEXT rather than default (#21031) Signed-off-by: Navneet Verma (cherry picked from commit 311d711f62bff4c36f895885c02dfa6f879cc56e) Signed-off-by: github-actions[bot] --- CHANGELOG.md | 1 + .../index/store/FsDirectoryFactory.java | 7 ++- .../index/store/FsDirectoryFactoryTests.java | 57 +++++++++++++++++++ 3 files changed, 64 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f766abb8cbfdf..e0063cd2b1e25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Move pull-based ingestion classes from experimental to publicAPI ([#20704](https://github.com/opensearch-project/OpenSearch/pull/20704)) - Lazy init stored field reader in SourceLookup ([#20827](https://github.com/opensearch-project/OpenSearch/pull/20827)) * Improved error message when trying to open an index originally created with Elasticsearch on OpenSearch ([#20512](https://github.com/opensearch-project/OpenSearch/pull/20512)) +- Updated MMapDirectory to use ReadAdviseByContext rather than default readadvise of Lucene([#21031](https://github.com/opensearch-project/OpenSearch/pull/21031)) ### Fixed - Relax index template pattern overlap check to use minimum-string heuristic, allowing distinguishable multi-wildcard patterns at the same priority ([#20702](https://github.com/opensearch-project/OpenSearch/pull/20702)) 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 4b23ca6b19d7c..784a8260858ad 100644 --- a/server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java +++ b/server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java @@ -100,12 +100,17 @@ public Directory newFSDirectory(Path location, LockFactory lockFactory, IndexSet final FSDirectory primaryDirectory = FSDirectory.open(location, lockFactory); final Set nioExtensions = new HashSet<>(indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS)); if (primaryDirectory instanceof MMapDirectory mMapDirectory) { + // Setting the read advise by context: REF: https://github.com/opensearch-project/OpenSearch/issues/21012 + mMapDirectory.setReadAdvice(MMapDirectory.ADVISE_BY_CONTEXT); return new HybridDirectory(lockFactory, setPreload(mMapDirectory, preLoadExtensions), nioExtensions); } else { return primaryDirectory; } case MMAPFS: - return setPreload(new MMapDirectory(location, lockFactory), preLoadExtensions); + final MMapDirectory mMapDirectory = new MMapDirectory(location, lockFactory); + // Setting the read advise by context: REF: https://github.com/opensearch-project/OpenSearch/issues/21012 + mMapDirectory.setReadAdvice(MMapDirectory.ADVISE_BY_CONTEXT); + return setPreload(mMapDirectory, preLoadExtensions); // simplefs was removed in Lucene 9; support for enum is maintained for bwc case SIMPLEFS: case NIOFS: 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 6354fa04b0063..6cda5224549da 100644 --- a/server/src/test/java/org/opensearch/index/store/FsDirectoryFactoryTests.java +++ b/server/src/test/java/org/opensearch/index/store/FsDirectoryFactoryTests.java @@ -31,14 +31,18 @@ package org.opensearch.index.store; +import org.apache.lucene.store.DataAccessHint; import org.apache.lucene.store.Directory; import org.apache.lucene.store.IOContext; import org.apache.lucene.store.MMapDirectory; +import org.apache.lucene.store.MergeInfo; import org.apache.lucene.store.NIOFSDirectory; import org.apache.lucene.store.NoLockFactory; +import org.apache.lucene.store.ReadAdvice; import org.apache.lucene.util.Constants; import org.opensearch.Version; import org.opensearch.cluster.metadata.IndexMetadata; +import org.opensearch.common.SuppressForbidden; import org.opensearch.common.settings.Settings; import org.opensearch.core.index.Index; import org.opensearch.core.index.shard.ShardId; @@ -49,11 +53,14 @@ import org.opensearch.test.OpenSearchTestCase; import java.io.IOException; +import java.lang.reflect.Field; import java.nio.file.Files; import java.nio.file.Path; import java.util.Arrays; import java.util.Locale; +import java.util.Optional; import java.util.Set; +import java.util.function.BiFunction; import java.util.function.BiPredicate; import static org.opensearch.test.store.MockFSDirectoryFactory.FILE_SYSTEM_BASED_STORE_TYPES; @@ -173,6 +180,7 @@ private void doTestStoreDirectory(Path tempDir, String typeSettingValue, IndexMo switch (type) { case HYBRIDFS: assertTrue(FsDirectoryFactory.isHybridFs(directory)); + mMapDirectoryHasReadAdviceByContext(((FsDirectoryFactory.HybridDirectory) directory).getDelegate()); break; // simplefs was removed in Lucene 9; support for enum is maintained for bwc case SIMPLEFS: @@ -181,6 +189,7 @@ private void doTestStoreDirectory(Path tempDir, String typeSettingValue, IndexMo break; case MMAPFS: assertTrue(type + " " + directory.toString(), directory instanceof MMapDirectory); + mMapDirectoryHasReadAdviceByContext((MMapDirectory) directory); break; case FS: if (Constants.JRE_IS_64BIT) { @@ -194,4 +203,52 @@ private void doTestStoreDirectory(Path tempDir, String typeSettingValue, IndexMo } } } + + @SuppressForbidden(reason = "Need to check the readAdvise as there is no getter on read advise") + private void mMapDirectoryHasReadAdviceByContext(MMapDirectory mapDirectory) { + try { + @SuppressWarnings("unchecked") + BiFunction> readAdvice = (BiFunction< + String, + IOContext, + Optional>) getReadAdviceField(mapDirectory); + + // Verify the function behaves identically to ADVISE_BY_CONTEXT + // ADVISE_BY_CONTEXT returns the ReadAdvice from the IOContext + assertEquals( + "Advise By context is not set", + MMapDirectory.ADVISE_BY_CONTEXT.apply("test.dvd", IOContext.DEFAULT), + readAdvice.apply("test.dvd", IOContext.DEFAULT) + ); + assertEquals( + "Advise By context is not set", + MMapDirectory.ADVISE_BY_CONTEXT.apply("test.tim", IOContext.READONCE), + readAdvice.apply("test.tim", IOContext.READONCE) + ); + MergeInfo mergeInfo = new MergeInfo(100, 100L, false, 1); + assertEquals( + "Advise By context is not set", + MMapDirectory.ADVISE_BY_CONTEXT.apply("test.vec", IOContext.merge(mergeInfo)), + readAdvice.apply("test.vec", IOContext.merge(mergeInfo).withHints()) + ); + + assertEquals( + "Advise By context is not set", + MMapDirectory.ADVISE_BY_CONTEXT.apply("test.vec", IOContext.DEFAULT.withHints(DataAccessHint.RANDOM)), + readAdvice.apply("test.vec", IOContext.DEFAULT.withHints(DataAccessHint.RANDOM)) + ); + + } catch (Exception e) { + throw new RuntimeException("Failed to verify read advice is set to ADVISE_BY_CONTEXT: ", e); + } + + } + + @SuppressForbidden(reason = "need reflection to access private readAdvice field for testing") + private Object getReadAdviceField(MMapDirectory mMapDirectory) throws Exception { + Field readAdviceField = MMapDirectory.class.getDeclaredField("readAdvice"); + readAdviceField.setAccessible(true); + return readAdviceField.get(mMapDirectory); + } + }