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

Update to Apache Lucene 9.11.0-snapshot-4be6531 #13850

Merged
merged 1 commit into from
May 28, 2024

Conversation

navneet1v
Copy link
Contributor

@navneet1v navneet1v commented May 27, 2024

Description

Update to Apache Lucene 9.11.0-snapshot-4be6531 for main branch only, as 9.11 version of Lucene is not released.

There are some changes that made some changes in highlighter classes. I have fixed those. One of the major was adding of a Passage Comparator. As of now I have used the default comparator
Comparator.comparingInt(Passage::getStartOffset) which was getting used earlier.
Ref: apache/lucene#13276

Related Issues

Resolves #13848

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • API changes companion pull request created.
  • Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  • Public documentation issue/PR created

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.

@navneet1v
Copy link
Contributor Author

@reta needs your review here.

Copy link
Contributor

❌ Gradle check result for 397ac41: FAILURE

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?

@reta
Copy link
Collaborator

reta commented May 27, 2024

@reta needs your review here.

Seems like it needs more work, Apache Lucene updates are rarely seamless ...

@navneet1v
Copy link
Contributor Author

@reta needs your review here.

Seems like it needs more work, Apache Lucene updates are rarely seamless ...

yeah.. fixing the issues now.

@navneet1v
Copy link
Contributor Author

@reta the whole idea of updating snapshot was to make this change: server/src/main/java/org/opensearch/plugins/PluginsService.java into Opensearch. What would be the recommendation here to add this SPI reloading change? Should I create a new PR or use the same PR? I want to ensure that whenever 9.11 is released the reloading of KNNVectorsFormat SPI also comes to 2.x branch.

@reta
Copy link
Collaborator

reta commented May 27, 2024

What would be the recommendation here to add this SPI reloading change? Should I create a new PR or use the same PR? I want to ensure that whenever 9.11 is released the reloading of KNNVectorsFormat SPI also comes to 2.x branch.

@navneet1v I think it depends on the change: if it is more or less trivial -we could do it in this pull request, if it needs some non-trivial code changes, having the separate pull request (and backport to 2.x once 9.11 is released) would be preferred.

@navneet1v
Copy link
Contributor Author

navneet1v commented May 27, 2024

What would be the recommendation here to add this SPI reloading change? Should I create a new PR or use the same PR? I want to ensure that whenever 9.11 is released the reloading of KNNVectorsFormat SPI also comes to 2.x branch.

@navneet1v I think it depends on the change: if it is more or less trivial -we could do it in this pull request, if it needs some non-trivial code changes, having the separate pull request (and backport to 2.x once 9.11 is released) would be preferred.

@reta
its a 1 line change in Opensearch. Now in terms of whether it is trivial or not depends on the reviewer. For me the change is similar to hitting the reloading SPI api of KNNVectorsFormat when plugins are loaded just like Opensearch do for Codecs, DocValues, PostingsFormat.

The reason why I was reluctant to raise another PR is, whenever backport of Lucene 9.11 version will happen in core I don't want that change to be missed as it will break k-NN plugin. Hence thinking to use this PR only. Any suggestions?

Copy link
Contributor

❕ Gradle check result for d18fb26: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Copy link

codecov bot commented May 27, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 71.57%. Comparing base (b15cb0c) to head (fe4edef).
Report is 313 commits behind head on main.

Files Patch % Lines
...etch/subphase/highlight/FastVectorHighlighter.java 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #13850      +/-   ##
============================================
+ Coverage     71.42%   71.57%   +0.15%     
- Complexity    59978    61289    +1311     
============================================
  Files          4985     5063      +78     
  Lines        282275   288030    +5755     
  Branches      40946    41710     +764     
============================================
+ Hits         201603   206167    +4564     
- Misses        63999    64875     +876     
- Partials      16673    16988     +315     

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

@reta
Copy link
Collaborator

reta commented May 27, 2024

The reason why I was reluctant to raise another PR is, whenever backport of Lucene 9.11 version will happen in core I don't want that change to be missed as it will break k-NN plugin. Hence thinking to use this PR only. Any suggestions?

Please raise the separate pull request, we will backport it, thank you.

@navneet1v navneet1v requested a review from reta May 28, 2024 07:03
Copy link
Contributor

❌ Gradle check result for fe4edef: FAILURE

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?

@reta
Copy link
Collaborator

reta commented May 28, 2024

❌ Gradle check result for fe4edef: FAILURE

java.lang.Exception: Suite timeout exceeded (>= 1200000 msec).
	at __randomizedtesting.SeedInfo.seed([966FE3B6231BC042]:0)

Copy link
Contributor

❌ Gradle check result for fe4edef: FAILURE

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?

@reta
Copy link
Collaborator

reta commented May 28, 2024

❌ Gradle check result for fe4edef: FAILURE

#9859

@reta reta added the dependencies Pull requests that update a dependency file label May 28, 2024
Copy link
Contributor

✅ Gradle check result for fe4edef: SUCCESS

@reta reta merged commit 45360db into opensearch-project:main May 28, 2024
34 of 35 checks passed
@navneet1v
Copy link
Contributor Author

@reta thanks for merging the code and running the gradle checks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement Enhancement or improvement to existing feature or request Other skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump Apache Lucene 9.11 snapshot in main to include commit 8d7bf86bbeccefe66ef67d424ffc748e56410e84
3 participants