Making version of lucene k-nn engine match lucene current version #691
Conversation
8a8ffab to
e6c69a5
Compare
|
|
||
| public void testVersion() { | ||
| Lucene luceneInstance = Lucene.INSTANCE; | ||
| assertEquals("9.5.0", luceneInstance.getVersion()); |
There was a problem hiding this comment.
This test will fail when OS upgrades to a new version. Let's not hard code this version. See if we can get this value from some other constant which is present in OS. So that when that constants updates the test uses the latest version.
I think you can use Version.LATEST.toString()
There was a problem hiding this comment.
Yes, it will fail on OS upgrade, I actually done it on purpose for us to make sure version upgrade doesn't flow unattended, like it was with 9.2 that made all the way up to 9.5. With variables it's harder to check which exact value it has.
There was a problem hiding this comment.
But here is the problem, if we have hard code the value as "9.5.0" as intentional, then in src file too we should make it 9.5.0 rather than Version.LATEST.toString(). Because the next time this Unit test will fail and then we need to take a decision whether to upgrade or not.
There was a problem hiding this comment.
I think we need to discuss if k-NN engine is different from codec. For codec now we may have 9.4 (through bwc package) but lucene library will be 9.5. Does it mean the whole lucene k-NN engine is 9.4 as well? Engine is kNN concept, we can change it as needed.
We use codec, reader, writer, format that are essentially coming from codec. But rest of classes are from current lucene version (9.5 in my example).
My opinion - version of codec and engine should match. IMO we should upgrade to latest codec from our lucene version (like 9.5 that is in 2.5 snapshot). But if can keep older codec versions then engine should remain 9.4 too.
@jmazanec15 what do you think?
There was a problem hiding this comment.
Here is our current guidance on Codec versioning: https://github.com/opensearch-project/k-NN/blob/main/DEVELOPER_GUIDE.md#codec-versioning.
Right now, we have 2 versioning systems - the first marks the codec version used for lucene, the second marks the version for k-NN. This is a bit confusing, however, I believe that the reasoning is so that we can properly read older segment files.
There was a problem hiding this comment.
@martin-gaievski on doing some deep-dive I understand the purpose of this code change better. We are updating the version of lucene, which is not related to codec over here.
My opinion - version of codec and engine should match. IMO we should upgrade to latest codec from our lucene version (like 9.5 that is in 2.5 snapshot). But if can keep older codec versions then engine should remain 9.4 too.
I agree that we should upgrade the codec version to include the latest codec. I will pick up that change.
But still my first comment hold over here. In the test we should not hard code string as "9.5.0", we should get this from Version.LATEST.toString(). Because this is unit test we want to make sure that if someone changes the value to other version which is not latest, in the src files tests should detect it. I don't feel comfortable in putting a hardcoded value in test. Here is another option we can explore, we can update the src file variable to take value from OpenSearch instead of Lucene. ref: https://github.com/opensearch-project/OpenSearch/blob/2.x/server/src/main/java/org/opensearch/Version.java#L108-L109
There was a problem hiding this comment.
ack, let me update to Version.LATEST
Codecov Report
@@ Coverage Diff @@
## main #691 +/- ##
============================================
- Coverage 84.46% 84.42% -0.05%
+ Complexity 1055 1053 -2
============================================
Files 149 149
Lines 4307 4307
Branches 385 385
============================================
- Hits 3638 3636 -2
- Misses 489 491 +2
Partials 180 180
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
93f987f to
efcd5c2
Compare
Signed-off-by: Martin Gaievski <gaievski@amazon.com>
efcd5c2 to
5841a20
Compare
Signed-off-by: Martin Gaievski <gaievski@amazon.com>
5841a20 to
689ca09
Compare
* Making version of lucene k-nn engine match lucene current version Signed-off-by: Martin Gaievski <gaievski@amazon.com> (cherry picked from commit 577205e)
Signed-off-by: Martin Gaievski gaievski@amazon.com
Description
Update version of Lucene kNN engine, it matches version of Lucene that is coming with core OpenSearch. Currently it's used as engine text description, it shouldn't cause bwc issues.
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.