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

Lucene 10 upgrade test fixes #3530

Conversation

rithin-pullela-aws
Copy link
Contributor

Description

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • 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.

@mingshl
Copy link
Collaborator

mingshl commented Feb 12, 2025

@peterzhuamazon can you please help review this PR? This should help with the lucene 10 upgrade and also update the alpha artifacts, thanks!

Copy link

codecov bot commented Feb 12, 2025

Codecov Report

Attention: Patch coverage is 38.46154% with 8 lines in your changes missing coverage. Please review.

Project coverage is 80.27%. Comparing base (d7dec0f) to head (2c07f31).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
...pensearch/ml/common/model/LocalRegexGuardrail.java 0.00% 2 Missing ⚠️
...nsearch/ml/autoredeploy/MLModelAutoReDeployer.java 0.00% 0 Missing and 2 partials ⚠️
...earch/ml/engine/indices/MLInputDatasetHandler.java 0.00% 1 Missing ⚠️
...action/undeploy/TransportUndeployModelsAction.java 0.00% 1 Missing ⚠️
...a/org/opensearch/ml/model/MLModelGroupManager.java 0.00% 0 Missing and 1 partial ⚠️
.../main/java/org/opensearch/ml/utils/IndexUtils.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3530      +/-   ##
============================================
+ Coverage     80.25%   80.27%   +0.02%     
- Complexity     6906     6908       +2     
============================================
  Files           610      610              
  Lines         30077    30081       +4     
  Branches       3368     3368              
============================================
+ Hits          24137    24148      +11     
+ Misses         4487     4482       -5     
+ Partials       1453     1451       -2     
Flag Coverage Δ
ml-commons 80.27% <38.46%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@rithin-pullela-aws rithin-pullela-aws requested a deployment to ml-commons-cicd-env-require-approval February 12, 2025 01:58 — with GitHub Actions Waiting
@Zhangxunmt
Copy link
Collaborator

LGTM, seems all required class path updates are included for Lucene 10.

From the codecov failure:
"plugin/src/main/java/org/opensearch/ml/utils/IndexUtils.java#L130
Added line #L130 was not covered by tests"

Can you add a test to cover this updated function call r.getHits().getTotalHits().value();?

@rithin-pullela-aws rithin-pullela-aws temporarily deployed to ml-commons-cicd-env-require-approval February 12, 2025 05:29 — with GitHub Actions Inactive
@rithin-pullela-aws rithin-pullela-aws had a problem deploying to ml-commons-cicd-env-require-approval February 12, 2025 05:29 — with GitHub Actions Failure
mingshl
mingshl previously approved these changes Feb 12, 2025
@rithin-pullela-aws rithin-pullela-aws had a problem deploying to ml-commons-cicd-env-require-approval February 12, 2025 17:25 — with GitHub Actions Failure
@@ -419,6 +419,7 @@ configurations.all {
resolutionStrategy.force "org.apache.logging.log4j:log4j-api:2.24.2"
resolutionStrategy.force "org.apache.logging.log4j:log4j-core:2.24.2"
resolutionStrategy.force "jakarta.json:jakarta.json-api:2.1.3"
resolutionStrategy.force "org.opensearch:opensearch:3.0.0-alpha1-SNAPSHOT"
Copy link
Member

@peterzhuamazon peterzhuamazon Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is hardcoding, you should use ${opensearch_version}.

@peterzhuamazon
Copy link
Member

peterzhuamazon commented Feb 12, 2025

Jacoco also needs to be 0.8.12 to support jdk23, which was initially added in this PR:

@mingshl mingshl dismissed their stale review February 12, 2025 17:47

seems some comments needed to address and those are addressed in #3426

@dhrubo-os
Copy link
Collaborator

Why are we duplicating the effort? When a PR is already raised from @zane-neo

@rithin-pullela-aws rithin-pullela-aws marked this pull request as draft February 12, 2025 19:54
@rithin-pullela-aws
Copy link
Contributor Author

Why are we duplicating the effort? When a PR is already raised from @zane-neo

We wanted to test the proposed changes and potentially have them merged. Unfortunately, we could not do that.

@dhrubo-os
Copy link
Collaborator

@rithin-pullela-aws do we need this PR anymore?

@rithin-pullela-aws
Copy link
Contributor Author

@rithin-pullela-aws do we need this PR anymore?

Closing this since #3426 is successfully merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants