-
Notifications
You must be signed in to change notification settings - Fork 136
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
Bumping main version to opensearch core 2.1.0 #411
Bumping main version to opensearch core 2.1.0 #411
Conversation
588a0a1
to
d26169c
Compare
Codecov Report
@@ Coverage Diff @@
## main #411 +/- ##
=========================================
Coverage 84.01% 84.01%
Complexity 911 911
=========================================
Files 130 130
Lines 3879 3879
Branches 359 359
=========================================
Hits 3259 3259
Misses 458 458
Partials 162 162
Continue to review full report at Codecov.
|
a8baf75
to
55f5cf4
Compare
Signed-off-by: Martin Gaievski <[email protected]>
55f5cf4
to
970dc05
Compare
@@ -44,6 +44,7 @@ apply from: 'gradle/formatting.gradle' | |||
apply plugin: 'opensearch.opensearchplugin' | |||
apply plugin: 'opensearch.rest-test' | |||
apply plugin: 'opensearch.pluginzip' | |||
apply plugin: 'opensearch.repositories' |
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.
Why we need this?
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 required to get some dependencies for Core OpenSearch, for instance for us it was failing without this plugin on getting proper lucene version, please see details here opensearch-project/OpenSearch#3455
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 this is the best way to get the lucene, what about the snapshot provided in the github issue?
maven { url "https://d1nvenhzbhpy0q.cloudfront.net/snapshots/lucene/" }
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.
both approaches recommended by Core team, my guess they are equivalent
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 post provides good explanation - opensearch-project/OpenSearch#2500 (comment)
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.
Please check the comment in build.gradle file, around why we need
apply plugin: 'opensearch.repositories'
@@ -51,7 +51,7 @@ testClusters { | |||
} | |||
|
|||
// Skip test if version is 1.0, 1.1, 1.2 or 1.3 as they are not supported in those versions | |||
if (knn_bwc_version.startsWith("1.")) { | |||
if (knn_bwc_version.startsWith("1.") || knn_bwc_version.startsWith("2.0")) { |
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.
Why not replace "1." with "2."? Are we expecting backward compatibility testing across main version?
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.
We are also running BWC Tests from 1.x versions to 2.x versions. So, we need to add that condition
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.
My understanding is we need both. opensearch_version defines current version, knn_bwc_version defines the version we want to be compatible with, so setting those to let's say 2.1.0 and 1.3.0 we testing if 2.1.0 knn is backward compatible with 1.3.0.
We are checking such compatibility for all versions defined here https://github.com/opensearch-project/k-NN/actions/runs/2398778347
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.
@VijayanB does it make sense? Actually we've added rolling upgrade bwc for both 1.3.2 and 2.0.0
Signed-off-by: Martin Gaievski <[email protected]>
cb192f3
to
2fecd57
Compare
Signed-off-by: Martin Gaievski [email protected]
Description
Upgrade core OpenSearch version to 2.1.0 after 2.0.0 GA release.
Issues Resolved
#385
Check List
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.