Default to mmapfs within hybridfs

[dzane17]

Description

Currently OpenSearch code contains an explicit list of file extensions which load using mmap from hybridfs. Other file extensions default to nio. This PR flips the logic to keep a list of nio file extensions while all others default to mmap. This will prevent any future regressions in case Lucene adds new segment file type that should be using mmap.

Related Issues

Resolves https://github.com/opensearch-project/OpenSearch/issues/8297

Check List

• [x] New functionality includes testing.
  • [x] All tests pass
• [x] New functionality has been documented.
  • [x] New functionality has javadoc added
• [x] Commits are signed per the DCO using --signoff
• [x] Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE :grey_exclamation:
• TEST FAILURES:

    1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testDropPrimaryDuringReplication
    1 org.opensearch.remotestore.RemoteStoreIT.testStaleCommitDeletionWithInvokeFlush

• URL: https://build.ci.opensearch.org/job/gradle-check/19335/
• CommitID: 541ebc708769ef755dc3158940330d9889694548 Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/19339/
• CommitID: d84844315bcb95f1e6d30534d98e0b9b80600531 Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE :grey_exclamation:
• TEST FAILURES:

    1 org.opensearch.remotestore.RemoteStoreIT.testStaleCommitDeletionWithoutInvokeFlush
    1 org.opensearch.indices.replication.SegmentReplicationIT.testScrollCreatedOnReplica

• URL: https://build.ci.opensearch.org/job/gradle-check/19441/
• CommitID: d84844315bcb95f1e6d30534d98e0b9b80600531 Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[codecov[bot]]

Codecov Report

Merging #8508 (b8e1e09) into main (4114009) will decrease coverage by 0.08%. Report is 4 commits behind head on main. The diff coverage is 86.84%.

@@             Coverage Diff              @@
##               main    #8508      +/-   ##
============================================
- Coverage     71.19%   71.12%   -0.08%     
- Complexity    57455    57476      +21     
============================================
  Files          4777     4777              
  Lines        270712   270746      +34     
  Branches      39566    39572       +6     
============================================
- Hits         192729   192555     -174     
- Misses        61782    62052     +270     
+ Partials      16201    16139      -62     

Files Changed	Coverage Δ
...pensearch/common/settings/IndexScopedSettings.java	100.00% <ø> (ø)
...rc/main/java/org/opensearch/index/IndexModule.java	82.35% <81.48%> (-0.14%)	:arrow_down:
...org/opensearch/index/store/FsDirectoryFactory.java	79.72% <100.00%> (+2.11%)	:arrow_up:

... and 461 files with indirect coverage changes

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE :grey_exclamation:
• TEST FAILURES:

    2 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testDropPrimaryDuringReplication
    1 org.opensearch.remotestore.multipart.RemoteStoreMultipartIT.testStaleCommitDeletionWithInvokeFlush

• URL: https://build.ci.opensearch.org/job/gradle-check/20070/
• CommitID: 967864437c720c8a8a9311cb00475d52240ddcf0 Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE :grey_exclamation:
• TEST FAILURES:

    1 org.opensearch.indices.replication.SegmentReplicationRelocationIT.testPrimaryRelocationWithSegRepFailure
    1 org.opensearch.indices.replication.SegmentReplicationRelocationIT.testPrimaryRelocation

• URL: https://build.ci.opensearch.org/job/gradle-check/20071/
• CommitID: 41e7d3cd07cdfd44eedc2f702d45e65a9519af60 Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/20160/
• CommitID: 311e98c7117cb451e702297f578865e90b3add20 Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS :white_check_mark:
• URL: https://build.ci.opensearch.org/job/gradle-check/20161/
• CommitID: b0a8594bd933e36aed1cb68807fdee4258d6a791

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE :grey_exclamation:
• TEST FAILURES:

    1 org.opensearch.remotestore.RemoteStoreIT.testStaleCommitDeletionWithInvokeFlush
    1 org.opensearch.indices.replication.SegmentReplicationIT.testScrollCreatedOnReplica

• URL: https://build.ci.opensearch.org/job/gradle-check/20162/
• CommitID: 7a245db05ad9976fa91b4739f3d19964fd2ee1a3 Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[jainankitk]

Couple of minor comments. LGTM otherwise!

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE :grey_exclamation:
• TEST FAILURES:

    2 org.opensearch.remotestore.multipart.RemoteStoreMultipartIT.testStaleCommitDeletionWithInvokeFlush
    1 org.opensearch.remotestore.RemoteStoreIT.testStaleCommitDeletionWithInvokeFlush
    1 org.opensearch.indices.replication.SegmentReplicationIT.testReplicationPostDeleteAndForceMerge
    1 org.opensearch.indices.replication.SegmentReplicationIT.testReplicaHasDiffFilesThanPrimary

• URL: https://build.ci.opensearch.org/job/gradle-check/20165/
• CommitID: c1467588764dc2b8e09b8cd7a4c74e1651505a97 Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE :grey_exclamation:
• TEST FAILURES:

    1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testPitCreatedOnReplica
    1 org.opensearch.remotestore.CreateRemoteIndexTranslogDisabledIT.testDefaultRemoteStoreNoUserOverride
    1 org.opensearch.indices.replication.SegmentReplicationIT.testScrollCreatedOnReplica
    1 org.opensearch.cluster.ClusterHealthIT.testHealthOnClusterManagerFailover

• URL: https://build.ci.opensearch.org/job/gradle-check/20313/
• CommitID: b76e6db3596baa1478b2a8ae1f4792eb1cd9ce00 Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[dzane17]

@reta any other comments?

[reta]

@reta any other comments?

Apologies, I was off last week

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/20943/
• CommitID: 7a24d12e8e27dec64d46c8b72dd87a4d87893352 Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE :grey_exclamation:
• TEST FAILURES:

    1 org.opensearch.indices.replication.SegmentReplicationTargetServiceTests.testShardAlreadyReplicating

• URL: https://build.ci.opensearch.org/job/gradle-check/20945/
• CommitID: 48315e0d63713d946d98c999e2f954e31c2ce575 Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[reta]

@andrross @dblock do you have any concerns with this change?

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE :grey_exclamation:
• TEST FAILURES:

    1 org.opensearch.search.suggest.CompletionSuggestSearchIT.testSkipDuplicates

• URL: https://build.ci.opensearch.org/job/gradle-check/20996/
• CommitID: 0a0b58fdc94075c85119eae6194479952eab8a8b Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[dzane17]

@reta What is the process for merging? Do we need additional reviews?

[reta]

@reta What is the process for merging? Do we need additional reviews?

I asked for some more 👀 here (https://github.com/opensearch-project/OpenSearch/pull/8508#issuecomment-1651932552), sensitive change

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/21841/
• CommitID: 6dbc66ed30a1c792b50419f17a4a9e33e4b3d0e1 Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[opensearch-trigger-bot[bot]]

Compatibility status:

> Task :checkCompatibility
Incompatible components: [https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/security-analytics.git]
Compatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git]

BUILD SUCCESSFUL in 26m 55s

[dzane17]

mmap.extensions is deprecated and commented here

[jainankitk]

mmap.extensions is deprecated and commented here

Sorry, my bad. Somehow missed that! The changes LGTM otherwise

[andrross]

mmap.extensions is deprecated and commented here

@dzane17 Added a comment in the code, but the settings framework has a mechanism for marking a setting as deprecated (separate from the Java code annotation).

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/22142/
• CommitID: 1d58967903b91771f2b5c06cb8bca20d266950d6 Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/22143/
• CommitID: 543e590df6b1eac510c96ff02078bd003a7ad036 Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/22140/
• CommitID: 47adabc40c2281468f9684c0751333757d2590c8 Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS :white_check_mark:
• URL: https://build.ci.opensearch.org/job/gradle-check/22792/
• CommitID: 543e590df6b1eac510c96ff02078bd003a7ad036

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/22825/
• CommitID: ea0fa6ad0c7e067d800a8323b4f6b24ba190b430 Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE :grey_exclamation:
• TEST FAILURES:

    1 org.opensearch.http.SearchRestCancellationIT.testAutomaticCancellationDuringFetchPhase

• URL: https://build.ci.opensearch.org/job/gradle-check/22832/
• CommitID: b8e1e09a2291f70d531ece0698fe245cf3f4f54d Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[navneet1v]

@andrross , @dzane17 after this change does it means all the files which are in this new list[INDEX_STORE_HYBRID_NIO_EXTENSIONS], will be mmaped or not?

[jainankitk]

@andrross , @dzane17 after this change does it means all the files which are in this new list[INDEX_STORE_HYBRID_NIO_EXTENSIONS], will be mmaped or not?

@navneet1v - All the files in new list [INDEX_STORE_HYBRID_NIO_EXTENSIONS], will NOT be mmapped. So, that in future if lucene adds new extension and we miss including that, it will be mmapped by default and no performance regression

[navneet1v]

@jainankitk can you help me understand this then, all the files which in this new list INDEX_STORE_HYBRID_NIO_EXTENSIONS, why they are not mmaped? I think mmap is a good optimization. Am I missing something here?

[martin-gaievski]

with changes in this PR "vec" and "vex" will be handled with "NIO" mode. Those are standard Lucene files for vector format, switching them to nio will introduce a performance regression

[dzane17]

@navneet1v @martin-gaievski This PR does not change existing file loading behavior. The only changes are:

1. New setting index.store.hybrid.nio.extensions. Should be used in place of index.store.hybrid.mmap.extensions
2. New file types added in the future default to MMAP instead of NIO

Old Logic

// default list of mmap extensions
INDEX_STORE_HYBRID_MMAP_EXTENSIONS = ["nvd", "dvd", "tim", "tip", "dim", "kdd", "kdi", "cfs", "doc"]

// any extensions not in INDEX_STORE_HYBRID_MMAP_EXTENSIONS list use nio
// This includes but is not limited to ["segments_N", "write.lock", "si", "cfe", "fnm", "fdx", "fdt", "pos", "pay", "nvm", "dvm", "tvx", "tvd", "liv", "dii", "vec", "vem", ...]

vec, vex were never part of INDEX_STORE_HYBRID_MMAP_EXTENSIONS so have been using NIO all long.

We can change which extensions use NIO/MMAP, but this PR does not cause any MORE files to use NIO than before.

[navneet1v]

@dzane17 earlier plugins had an opportunity to add files which can be mmaped. Now as this setting INDEX_STORE_HYBRID_MMAP_EXTENSIONS is marked as deprecated and new setting index.store.hybrid.nio.extensions is not mmaping the file. I want to know if a plugin wants add files that needs to mmaped how it can be done? Our unit tests are failing because INDEX_STORE_HYBRID_MMAP_EXTENSIONS is marked as deprecated leading to warning and failing the tests.

A simple question I want to ask is if I have file format that I want to be MMap, and is present in nio settings what should I do?

[martin-gaievski]

since lucene 9.4 performance of vector searches degraded and we had to fix it in knn 2.5 by adding "vec" and "vex" to list of mmaps explicitly. With current PR that fix doesn't work anymore.

[dzane17]

Adding a file extension to index.store.hybrid.mmap.extensions will still force it use MMAP. The setting has the deprecation tag but still functions the same.

If deprecation warning is causing issue, use index.store.hybrid.nio.extensions instead. Files in nio.extensions will use NIO. All other files will use MMAP.

So if you want vec to be MMAPed. Pass a list WITHOUT vec to nio.extensions for example: ["segments_N", "write.lock", "si", "cfe", "fnm", "fdx", "fdt", "pos", "pay", "nvm", "dvm", "tvx", "tvd", "liv", "dii", "vem"]

Note: mmap.extensions & nio.extensions cannot both be set

[navneet1v]

@dzane17 what you explained on top is the performance degradation. Like if .vec file extensions was not present in OpenSearch core MMAP file extensions list so why we added it in NIO extensions? That is not clear.

[jainankitk]

@navneet1v - If you have already verified that vec and vem files performance is degraded upon NIO, you can raise PR to:

• remove those files from this default NIO list. Then you don't need the override in knn plugin as well. OR
• Another option is to use the nio list instead of mmap in knn plugin

Ideally, it should have been done in the core to begin with. The default opensearch core behavior has not changed at all by this PR.

[navneet1v]

The default opensearch core behavior has not changed at all by this PR.

@jainankitk I disagree here. First thing, why we don't want to map files as MMap. Secondly, we should either be calling out that OpenSearch min distribution behavior is not changed. Rather than saying opensearch core behavior has not.

and 1 more thing, Lucene also has .vex file which with the current code change is MMapped. If we really want to say there is no diff in min distribution behavior then .vex file should also be part of nio list.

So, I would say that this change is not consistent.

Lucene 9.5: https://lucene.apache.org/core/9_5_0/core/org/apache/lucene/codecs/lucene95/Lucene95HnswVectorsFormat.html

Also, how we did the performance testing to validate that after this change there is no performance degradation?

[jainankitk]

First thing, why we don't want to map files as MMap.

Because that's how the hybrid directory works. Files NOT present in the MMap list get used as niofs. This has been the behavior since Elasticsearch 7.0. Are you saying everything should mmapped?

Secondly, we should either be calling out that OpenSearch min distribution behavior is not changed. Rather than saying opensearch core behavior has not.

It is hard to keep track of code/plugins outside of core. Ideally if knn plugin has this hard plugin, there should be unit test in core to that effect

and 1 more thing, Lucene also has .vex file which with the current code change is MMapped. If we really want to say there is no diff in min distribution behavior then .vex file should also be part of nio list.

Thank you for pointing this out. @dzane17 - Can you raise PR for including vex extension as well? Also, please ensure no other extensions are missed

Also, how we did the performance testing to validate that after this change there is no performance degradation?

The default behavior has not been modified with this change. Performance testing happens as part of the nightly benchmarks - https://opensearch.org/benchmarks/. Are there nightly benchmarks for knn plugin that show regression? If yes, can we include it here for better tracking?

[navneet1v]

Thank you for pointing this out. @dzane17 - Can you raise PR for including vex extension as well? Also, please ensure no other extensions are missed

I would say rather than raising that PR to add lets do a PR to remove the .vec file from the nio list. Why cannot we do that?

@dzane17 @jainankitk

[navneet1v]

@jainankitk we have separate K-NN benchmarks. We saw the performance improvements here: https://github.com/opensearch-project/k-NN/pull/721 when we included the vec and vex file as MMapped files

[jainankitk]

@jainankitk we have separate K-NN benchmarks. We saw the performance improvements here: https://github.com/opensearch-project/k-NN/pull/721 when we included the vec and vex file as MMapped files

Do we have nightly run for those benchmarks? If yes, can you share dashboard for the same? Either way, can you create github issue for tracking the performance benchmark for "OpenSearch min distribution" and not just core.

I would say rather than raising that PR to add lets do a PR to remove the .vec file from the nio list. Why cannot we do that?

We can definitely do that as called out above as well - "remove those files from this default NIO list. Then you don't need the override in knn plugin as well." It is better if you can raise the PR for removing both from knn plugin and core for consistency. Else, you can take the below approach for knn plugin only fix.

A simple question I want to ask is if I have file format that I want to be MMap, and is present in nio settings what should I do?

Sorry missed this question earlier. It is pretty straightforward actually. You can get the default list of nio settings, remove the file extensions you want to mmap, set that as new value of nio extension file list. Essentially, New NIO list value = Set A (existing/default NIO list) - Set B (extensions to be mmapped). I hope that makes sense!

[navneet1v]

It is better if you can raise the PR for removing both from knn plugin and core for consistency

@jainankitk as per the previous response from "@dzane17 - Can you raise PR for including vex extension as well? Also, please ensure no other extensions are missed", if someone is adding the vex file, I was saying why to add, lets just remove the vec file from the nio list.

[jainankitk]

@jainankitk as per the previous response from "@dzane17 - Can you raise PR for including vex extension as well? Also, please ensure no other extensions are missed", if someone is adding the vex file, I was saying why to add, lets just remove the vec file from the nio list.

@navneet1v - It is okay for @dzane17 to fix the default behavior which was expected from this change. But, does not make sense to do changes not intended as part of this PR.

Also, there is knn plugin only option without changing the core at all. Essentially, New NIO list value = Set A (existing/default NIO list) - Set B (extensions to be mmapped).

[navneet1v]

@jainankitk the problem with @dzane17 changes would be that I need to revert it. The k-NN plugin option is not a good option, as in future if another behavior change happen then problem will arise.

[jainankitk]

@jainankitk the problem with @dzane17 changes would be that I need to revert it.

@navneet1v - I am not sure if I completely understand this. Do you mean completely revert this PR?