-
Notifications
You must be signed in to change notification settings - Fork 146
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
Fix test failure after lucene upgrade to 10 #3426
Fix test failure after lucene upgrade to 10 #3426
Conversation
Signed-off-by: zane-neo <[email protected]>
Signed-off-by: zane-neo <[email protected]>
Signed-off-by: zane-neo <[email protected]>
Signed-off-by: zane-neo <[email protected]>
build.gradle
Outdated
@@ -25,7 +25,7 @@ buildscript { | |||
opensearch_build += "-SNAPSHOT" | |||
} | |||
|
|||
common_utils_version = System.getProperty("common_utils.version", opensearch_build) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should stay on opensearch_build
as it will be switched to 3.0.0.0-alpha1-SNAPSHOT
as the version of common-utils.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterzhuamazon I didn't see the 3.0.0.0-alpha1-SNAPSHOT available on maven repo: https://aws.oss.sonatype.org/content/repositories/snapshots/org/opensearch/common-utils/, I can't compile success from my local.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's still failing with opensearch_build
for neural as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is because Common Utils owner needs to update their plugins 1st to be compatible with lucene10.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zane-neo @martin-gaievski I have updated common-utils main to 3.0.0.0-alpha1-SNAPSHOT and published to sonatype:
Thanks.
@@ -11,8 +11,8 @@ buildscript { | |||
ext { | |||
opensearch_group = "org.opensearch" | |||
isSnapshot = "true" == System.getProperty("build.snapshot", "true") | |||
opensearch_version = System.getProperty("opensearch.version", "3.0.0-SNAPSHOT") | |||
buildVersionQualifier = System.getProperty("build.version_qualifier", "") | |||
opensearch_version = System.getProperty("opensearch.version", "3.0.0-alpha1-SNAPSHOT") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is correct.
plugin/build.gradle
Outdated
@@ -56,7 +56,7 @@ dependencies { | |||
implementation group: 'org.opensearch', name: 'opensearch', version: "${opensearch_version}" | |||
implementation "org.opensearch.client:opensearch-rest-client:${opensearch_version}" | |||
// Multi-tenant SDK Client | |||
implementation "org.opensearch:opensearch-remote-metadata-sdk:${opensearch_build}" | |||
implementation "org.opensearch:opensearch-remote-metadata-sdk:3.0.0-SNAPSHOT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remote sdk should also be 3.0.0.0-alpha1-SNAPSHOT
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remote sdk should also be
3.0.0.0-alpha1-SNAPSHOT
.
We are still making changes on that for 2.19.0. I do not plan on updating to alpha until after at least code freeze date.
If anyone wants to get ahead of the game they are welcome to check out that repo and publish it locally with alpha1. All this churn as we're trying to finish up features within the week is causing a lot of wasted effort and delays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbwiddis just an update CU main is updated to 3.0.0.0-alpha1-SNAPSHOT:
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes for lucene upgrade (TotalHits) looks good
Build is fixed, need to change all the rest client calls back to normal client, instead of transport.client for IT test to pass. Think @rithin-pullela-aws will take a look on it per offline discussion. Thanks. |
Signed-off-by: zane-neo <[email protected]>
The failure test seems a flaky one, please review again |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3426 +/- ##
============================================
+ Coverage 80.25% 80.26% +0.01%
+ Complexity 6906 6904 -2
============================================
Files 610 610
Lines 30077 30077
Branches 3368 3368
============================================
+ Hits 24137 24141 +4
+ Misses 4487 4484 -3
+ Partials 1453 1452 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@zane-neo please refer to this change to address the code coverage issue
|
@@ -228,17 +228,13 @@ default Object getModelOutputValue(MLOutput mlOutput, String modelOutputFieldNam | |||
static Object parseDataInTensor(ModelTensor tensor) { | |||
Object modelOutputValue; | |||
if (tensor.getDataType().isInteger()) { | |||
modelOutputValue = Arrays.stream(tensor.getData()).map(Number::intValue).map(Integer::new).collect(Collectors.toList()); | |||
modelOutputValue = Arrays.stream(tensor.getData()).map(Number::intValue).collect(Collectors.toList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is removing .map(Integer::new)
necessary for lucene 10 changes?
(Same for all the changes in this file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JDK21 has removed the new Integer()
method, and based on the code, these conversion are not necessary so it's safe to remove all of them.
Signed-off-by: rithin-pullela-aws <[email protected]>
Update @rithin-pullela-aws test coverage commit to this PR. |
Signed-off-by: Peter Zhu <[email protected]>
Seems like integTest is not correctly using 3.0.0-alpha1-SNAPSHOT
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve based on the windows test passing, as well as unblock other plugins that depend on ml plugin.
Thanks everyone chiming in and help!
Description
After this change: opensearch-project/OpenSearch#16366, the TotalHits's value is been moved to a private field, unable to access it directly outside that class, so removing this to fix the test failure.
We need to change the
.value
to.value()
to make sure the code runs correctly.Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
--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.