Make HybridDirectory MMAP Extensions Configurable #3837

mattweber · 2022-07-09T23:42:16Z

Make the HybridDirectory's list of mmap extensions configurable
via index settings instead of being hard-coded.
Set defaults to the list of currently hard-coded values for
backwards compatibility.

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

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 · 2022-07-10T00:08:28Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/362/
CommitID: 114c39b99132bcc105145515255bc00fed2e0962

mattweber · 2022-07-10T14:36:20Z

@reta can you please rerun the tests, failure seems unrelated so I opened #3838.

reta · 2022-07-10T14:53:55Z

@reta can you please rerun the tests, failure seems unrelated so I opened #3838.

@mattweber I believe we need to get #3807 first, working on it

github-actions · 2022-07-10T14:56:43Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/363/
CommitID: 114c39b99132bcc105145515255bc00fed2e0962

mattweber · 2022-07-10T18:54:17Z

thanks @reta

reta · 2022-07-10T23:14:33Z

thanks @reta

@mattweber could you rebase please?

Make the HybridDirectory's list of mmap extensions configurable via index settings instead of being hard-coded to a specfic set. Set defaults to the list of currently hard-coded values for backwards compatibility. Signed-off-by: Matt Weber <[email protected]>

github-actions · 2022-07-11T01:21:56Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/370/
CommitID: 8a264c5

codecov-commenter · 2022-07-11T01:22:35Z

Codecov Report

Merging #3837 (8a264c5) into main (bfac8fc) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #3837      +/-   ##
============================================
+ Coverage     70.52%   70.60%   +0.07%     
- Complexity    56655    56673      +18     
============================================
  Files          4557     4557              
  Lines        272682   272685       +3     
  Branches      40038    40037       -1     
============================================
+ Hits         192310   192522     +212     
+ Misses        64203    63915     -288     
- Partials      16169    16248      +79

Impacted Files	Coverage Δ
...pensearch/common/settings/IndexScopedSettings.java	`100.00% <ø> (ø)`
...rc/main/java/org/opensearch/index/IndexModule.java	`82.03% <100.00%> (+0.32%)`	⬆️
...org/opensearch/index/store/FsDirectoryFactory.java	`77.61% <100.00%> (ø)`
...pensearch/client/cluster/RemoteConnectionInfo.java	`0.00% <0.00%> (-73.18%)`	⬇️
...a/org/opensearch/client/cluster/SniffModeInfo.java	`0.00% <0.00%> (-58.83%)`	⬇️
...a/org/opensearch/client/cluster/ProxyModeInfo.java	`0.00% <0.00%> (-55.00%)`	⬇️
.../opensearch/client/indices/CloseIndexResponse.java	`42.50% <0.00%> (-48.75%)`	⬇️
...opensearch/persistent/PersistentTasksExecutor.java	`22.22% <0.00%> (-44.45%)`	⬇️
.../search/aggregations/pipeline/HoltLinearModel.java	`20.33% <0.00%> (-42.38%)`	⬇️
.../opensearch/client/cluster/RemoteInfoResponse.java	`61.53% <0.00%> (-38.47%)`	⬇️
... and 495 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfac8fc...8a264c5. Read the comment docs.

reta · 2022-07-12T19:37:29Z

server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java

@@ -97,9 +97,10 @@ protected Directory newFSDirectory(Path location, LockFactory lockFactory, Index
            case HYBRIDFS:
                // Use Lucene defaults
                final FSDirectory primaryDirectory = FSDirectory.open(location, lockFactory);
+                final Set<String> mmapExtensions = new HashSet<>(indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS));


@mattweber I am trying to wrap my head around these two properties INDEX_STORE_PRE_LOAD_SETTING and INDEX_STORE_HYBRID_MMAP_EXTENSIONS, which apparently serve the same purpose (and do have same set of values), the HybridDirectory is looking like PreLoadMMapDirectory (except the setPreload thing) ... why do we need both?

@reta INDEX_STORE_PRE_LOAD_SETTING controls which files automatically get loaded into os cache and I don't believe this has any defaults as it is an expensive operation. This PR adds INDEX_STORE_HYBRID_MMAP_EXTENSIONS which controls which extensions are with opened with mmap store. The current defaults were picked by elastic after doing benchmarks. I would like it configurable so a user can tune to their specific usecase. For example, I want to mmap everything EXCEPT stored fields. The settings are related, but not the same.

mattweber · 2022-07-13T13:59:36Z

Thanks @reta! @dblock what do you think about this?

dblock · 2022-07-14T17:13:55Z

I know nothing about this stuff, let's have @mch2 take a look for a second pair of 👀 ?

nknize · 2022-07-14T17:42:42Z

server/src/main/java/org/opensearch/index/IndexModule.java

@@ -148,6 +148,14 @@ public final class IndexModule {
        Property.NodeScope
    );

+    public static final Setting<List<String>> INDEX_STORE_HYBRID_MMAP_EXTENSIONS = Setting.listSetting(


This is an expert parameter which, I suspect, few are going to touch. Can we javadoc this as expert with a description similar to above?

@nknize sure can

nknize · 2022-07-14T17:43:20Z

server/src/main/java/org/opensearch/index/IndexModule.java

+        List.of("nvd", "dvd", "tim", "tip", "dim", "kdd", "kdi", "cfs", "doc"),
+        Function.identity(),
+        Property.IndexScope,
+        Property.NodeScope


Do we also want to set this as a final setting (Property.Final)?

@nknize I don't think so, I would want to be able to change this by closing index, updating setting, then reopening. I thought final would prevent that scenario, but I could be wrong since I am not super familiar with that setting.

It would prevent that scenario. +1 to go w/o final.

Signed-off-by: Matt Weber <[email protected]>

github-actions · 2022-07-14T20:51:18Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/634/
CommitID: c36a65d

mattweber · 2022-07-14T22:15:36Z

@nknize @reta please kick off tests again, unrelated failures

github-actions · 2022-07-15T12:14:29Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/683/
CommitID: c36a65d

mattweber · 2022-07-20T18:46:29Z

@nknize @mch2 see any issues with this? Hope to get it in and backported to 2x so it will make one of the next releases. Thanks.

nknize

LGTM! Sorry for the hold up.

nknize · 2022-07-21T00:23:17Z

server/src/main/java/org/opensearch/index/IndexModule.java

+        List.of("nvd", "dvd", "tim", "tip", "dim", "kdd", "kdi", "cfs", "doc"),
+        Function.identity(),
+        Property.IndexScope,
+        Property.NodeScope


It would prevent that scenario. +1 to go w/o final.

mch2

Sorry had to do some homework to understand the settings. I have no concerns.

* Make HybridDirectory MMAP Extensions Configurable Make the HybridDirectory's list of mmap extensions configurable via index settings instead of being hard-coded to a specfic set. Set defaults to the list of currently hard-coded values for backwards compatibility. Signed-off-by: Matt Weber <[email protected]> * Add javadoc to INDEX_STORE_HYBRID_MMAP_EXTENSIONS. Signed-off-by: Matt Weber <[email protected]> (cherry picked from commit b08a2b8)

* Make HybridDirectory MMAP Extensions Configurable Make the HybridDirectory's list of mmap extensions configurable via index settings instead of being hard-coded to a specfic set. Set defaults to the list of currently hard-coded values for backwards compatibility. Signed-off-by: Matt Weber <[email protected]> * Add javadoc to INDEX_STORE_HYBRID_MMAP_EXTENSIONS. Signed-off-by: Matt Weber <[email protected]> (cherry picked from commit b08a2b8) Co-authored-by: Matt Weber <[email protected]>

mattweber requested review from a team and reta as code owners July 9, 2022 23:42

mattweber mentioned this pull request Jul 10, 2022

[BUG] gradle check not generaating expected file #3838

Closed

mattweber force-pushed the configurable_hybrid_store branch from 114c39b to 8a264c5 Compare July 11, 2022 00:57

dblock requested a review from mch2 July 11, 2022 21:48

reta reviewed Jul 12, 2022

View reviewed changes

reta approved these changes Jul 12, 2022

View reviewed changes

nknize requested changes Jul 14, 2022

View reviewed changes

Add javadoc to INDEX_STORE_HYBRID_MMAP_EXTENSIONS.

c36a65d

Signed-off-by: Matt Weber <[email protected]>

nknize approved these changes Jul 21, 2022

View reviewed changes

mch2 approved these changes Jul 21, 2022

View reviewed changes

dblock merged commit b08a2b8 into opensearch-project:main Jul 21, 2022

dblock added backport 2.x Backport to 2.x branch documentation pending Tracks issues which have PRs merged but documentation changes pending labels Jul 21, 2022

opensearch-trigger-bot bot mentioned this pull request Jul 21, 2022

[Backport 2.x] Make HybridDirectory MMAP Extensions Configurable #3970

Merged

jainankitk mentioned this pull request Jun 27, 2023

[BUG] Default to mmapfs within hybridfs #8297

Closed

nknize mentioned this pull request Aug 28, 2023

Default to mmapfs within hybridfs #8508

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make HybridDirectory MMAP Extensions Configurable #3837

Make HybridDirectory MMAP Extensions Configurable #3837

mattweber commented Jul 9, 2022

github-actions bot commented Jul 10, 2022

mattweber commented Jul 10, 2022

reta commented Jul 10, 2022

github-actions bot commented Jul 10, 2022

mattweber commented Jul 10, 2022

reta commented Jul 10, 2022

github-actions bot commented Jul 11, 2022

codecov-commenter commented Jul 11, 2022

reta Jul 12, 2022 •

edited

Loading

mattweber Jul 12, 2022

mattweber commented Jul 13, 2022

dblock commented Jul 14, 2022

nknize Jul 14, 2022

mattweber Jul 14, 2022

nknize Jul 14, 2022

mattweber Jul 14, 2022

nknize Jul 21, 2022

github-actions bot commented Jul 14, 2022

mattweber commented Jul 14, 2022

github-actions bot commented Jul 15, 2022

mattweber commented Jul 20, 2022

nknize left a comment

nknize Jul 21, 2022

mch2 left a comment

Make HybridDirectory MMAP Extensions Configurable #3837

Make HybridDirectory MMAP Extensions Configurable #3837

Conversation

mattweber commented Jul 9, 2022

Check List

github-actions bot commented Jul 10, 2022

Gradle Check (Jenkins) Run Completed with:

mattweber commented Jul 10, 2022

reta commented Jul 10, 2022

github-actions bot commented Jul 10, 2022

Gradle Check (Jenkins) Run Completed with:

mattweber commented Jul 10, 2022

reta commented Jul 10, 2022

github-actions bot commented Jul 11, 2022

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Jul 11, 2022

Codecov Report

reta Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

mattweber Jul 12, 2022

Choose a reason for hiding this comment

mattweber commented Jul 13, 2022

dblock commented Jul 14, 2022

nknize Jul 14, 2022

Choose a reason for hiding this comment

mattweber Jul 14, 2022

Choose a reason for hiding this comment

nknize Jul 14, 2022

Choose a reason for hiding this comment

mattweber Jul 14, 2022

Choose a reason for hiding this comment

nknize Jul 21, 2022

Choose a reason for hiding this comment

github-actions bot commented Jul 14, 2022

Gradle Check (Jenkins) Run Completed with:

mattweber commented Jul 14, 2022

github-actions bot commented Jul 15, 2022

Gradle Check (Jenkins) Run Completed with:

mattweber commented Jul 20, 2022

nknize left a comment

Choose a reason for hiding this comment

nknize Jul 21, 2022

Choose a reason for hiding this comment

mch2 left a comment

Choose a reason for hiding this comment

reta Jul 12, 2022 •

edited

Loading