Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove mmap.extensions setting #9392

Merged
merged 2 commits into from
Jul 29, 2024
Merged

Conversation

dzane17
Copy link
Contributor

@dzane17 dzane17 commented Aug 16, 2023

Description

Removes index.store.hybrid.mmap.extensions setting in OS 3.0.
Follow up to #8508

Background

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 #8297

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
  • 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
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:

Checks if related components are compatible with change 8d153ab

Incompatible components

Incompatible components: [https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/security-analytics.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/sql.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/geospatial.git, https://github.com/opensearch-project/cross-cluster-replication.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.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git]

@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:

Checks if related components are compatible with change 8d153ab

Incompatible components

Incompatible components: [https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/security-analytics.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/sql.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/geospatial.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/performance-analyzer.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]

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.smoketest.SmokeTestMultiNodeClientYamlTestSuiteIT.test {yaml=pit/10_basic/Delete all}

@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.80%. Comparing base (95fe9cb) to head (4c59590).
Report is 3 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #9392      +/-   ##
============================================
- Coverage     71.86%   71.80%   -0.07%     
+ Complexity    62757    62726      -31     
============================================
  Files          5161     5161              
  Lines        294370   294338      -32     
  Branches      42579    42574       -5     
============================================
- Hits         211548   211340     -208     
- Misses        65386    65510     +124     
- Partials      17436    17488      +52     

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

@jainankitk
Copy link
Collaborator

@dzane17 - Please update the change description, and get this merged with help from @andrross or @reta or one of the other maintainers. Although I don't see any backport label, still ensure to not backport into 2.x.

@reta reta added the v3.0.0 Issues and PRs related to version 3.0.0 label Jul 29, 2024
@andrross
Copy link
Member

@dzane17 @jainankitk This will break k-NN. We should update ensure k-NN gets updated before merging this change (either open an issue with a specific suggestion or create a PR directly).

Copy link
Contributor

❌ Gradle check result for bc0d3dd: null

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?

@jainankitk
Copy link
Collaborator

@dzane17 @jainankitk This will break k-NN. We should update ensure k-NN gets updated before merging this change (either open an issue with a specific suggestion or create a PR directly).

@navneet1v - Is this the only place KNN plugin is using this setting? Also, are there any file extensions that KNN needs to read as NIOFS? If not, MMAP is the new default unless overridden using NIOFS setting. cc: @vamshin

CHANGELOG-3.0.md Outdated Show resolved Hide resolved
Co-authored-by: Andriy Redko <[email protected]>
Signed-off-by: David Zane <[email protected]>
@reta reta mentioned this pull request Jul 29, 2024
20 tasks
@navneet1v
Copy link
Contributor

@dzane17 @jainankitk This will break k-NN. We should update ensure k-NN gets updated before merging this change (either open an issue with a specific suggestion or create a PR directly).

@navneet1v - Is this the only place KNN plugin is using this setting? Also, are there any file extensions that KNN needs to read as NIOFS? If not, MMAP is the new default unless overridden using NIOFS setting. cc: @vamshin

KNN don't use any files as NIOFS.

@jainankitk
Copy link
Collaborator

jainankitk commented Jul 29, 2024

KNN don't use any files as NIOFS.

@navneet1v - Great, thanks for confirming. Can you also create PR to remove the additionalSettings method from KNN plugin for 3.0? We can merge both the PRs together into main branch.

Copy link
Contributor

✅ Gradle check result for 4c59590: SUCCESS

@reta
Copy link
Collaborator

reta commented Jul 29, 2024

@andrross LGTY?

@andrross andrross merged commit 6dbb079 into opensearch-project:main Jul 29, 2024
33 of 34 checks passed
heemin32 added a commit to heemin32/k-NN that referenced this pull request Jul 29, 2024
Per opensearch-project/OpenSearch#9392, mmap will be
a default setting for all lucene segment files

Signed-off-by: Heemin Kim <[email protected]>
heemin32 added a commit to heemin32/k-NN that referenced this pull request Jul 29, 2024
Per opensearch-project/OpenSearch#9392, mmap will be
a default setting for all lucene segment files

Signed-off-by: Heemin Kim <[email protected]>
heemin32 added a commit to opensearch-project/k-NN that referenced this pull request Jul 29, 2024
Per opensearch-project/OpenSearch#9392, mmap will be
a default setting for all lucene segment files

Signed-off-by: Heemin Kim <[email protected]>
@dzane17 dzane17 deleted the mmap-2 branch July 30, 2024 17:50
harshavamsi pushed a commit to harshavamsi/OpenSearch that referenced this pull request Aug 20, 2024
* 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 pushed a commit to wdongyu/OpenSearch that referenced this pull request Aug 22, 2024
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working distributed framework v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Default to mmapfs within hybridfs
6 participants