Make HybridDirectory MMAP Extensions Configurable by mattweber · Pull Request #3837 · opensearch-project/OpenSearch

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 <matt@mattweber.org>

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

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.48%. Comparing base (bfac8fc) to head (c36a65d).
⚠️ Report is 4799 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3837      +/-   ##
============================================
- Coverage     70.52%   70.48%   -0.04%     
+ Complexity    56655    56638      -17     
============================================
  Files          4557     4557              
  Lines        272682   272685       +3     
  Branches      40038    40037       -1     
============================================
- Hits         192310   192215      -95     
- Misses        64203    64285      +82     
- Partials      16169    16185      +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

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

            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

        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 <matt@mattweber.org>

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 <matt@mattweber.org> * Add javadoc to INDEX_STORE_HYBRID_MMAP_EXTENSIONS. Signed-off-by: Matt Weber <matt@mattweber.org> (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 <matt@mattweber.org> * Add javadoc to INDEX_STORE_HYBRID_MMAP_EXTENSIONS. Signed-off-by: Matt Weber <matt@mattweber.org> (cherry picked from commit b08a2b8) Co-authored-by: Matt Weber <matt@mattweber.org>

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 suggested changes Jul 14, 2022

View reviewed changes

Add javadoc to INDEX_STORE_HYBRID_MMAP_EXTENSIONS.

c36a65d

Signed-off-by: Matt Weber <matt@mattweber.org>

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

Conversation

mattweber commented Jul 9, 2022

Check List

Uh oh!

github-actions bot commented Jul 10, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

mattweber commented Jul 10, 2022

Uh oh!

reta commented Jul 10, 2022

Uh oh!

github-actions bot commented Jul 10, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

mattweber commented Jul 10, 2022

Uh oh!

reta commented Jul 10, 2022

Uh oh!

github-actions bot commented Jul 11, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

codecov-commenter commented Jul 11, 2022 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

reta Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattweber Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

mattweber commented Jul 13, 2022

Uh oh!

dblock commented Jul 14, 2022

Uh oh!

nknize Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

mattweber Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

nknize Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

mattweber Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

nknize Jul 21, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 14, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

mattweber commented Jul 14, 2022

Uh oh!

github-actions bot commented Jul 15, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

mattweber commented Jul 20, 2022

Uh oh!

nknize left a comment

Choose a reason for hiding this comment

Uh oh!

nknize Jul 21, 2022

Choose a reason for hiding this comment

Uh oh!

mch2 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-commenter commented Jul 11, 2022 •

edited by codecov bot

Loading

reta Jul 12, 2022 •

edited

Loading