k-NN
k-NN copied to clipboard
Add basic integration test for segment replication
Description
This PR adds an integration test to do sanity for segment replication.
Issues Resolved
https://github.com/opensearch-project/k-NN/issues/921
Check List
- [ ] New functionality includes testing.
- [ ] All tests pass
- [ ] New functionality has been documented.
- [ ] New functionality has javadoc added
- [x] Commits are signed as per the DCO using --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.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
9e4251e) 84.92% compared to head (a364584) 85.10%. Report is 1 commits behind head on main.
:exclamation: Current head a364584 differs from pull request most recent head eb5fe85. Consider uploading reports for the commit eb5fe85 to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@
## main #927 +/- ##
============================================
+ Coverage 84.92% 85.10% +0.18%
+ Complexity 1274 1084 -190
============================================
Files 167 152 -15
Lines 5186 4404 -782
Branches 491 389 -102
============================================
- Hits 4404 3748 -656
+ Misses 574 479 -95
+ Partials 208 177 -31
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Is this the breaking change? Seems it doesn't work with rolling upgrades, or that is work in progress?
Thank you @martin-gaievski for reviewing this PR.
This PR is work related to bug on core which impacts OS 2.7.0 version. These tests are added to provide basic validation that kNN indices works with segment replication. These tests will also supplement testing for future changes on core around segment replication feature. One such example is support of rolling upgrades https://github.com/opensearch-project/OpenSearch/issues/3881
Added more details in https://github.com/opensearch-project/k-NN/pull/927
Are you going to add more tests? Seems this one uses basic settings: 1 shard, 1 replica. Also do we need to check scores and order of doc ids in result? I'm not sure what can be a side-effect of segment replication not working correctly.
Yes, I plan to add bwc tests as follow up of this PR. With segment replication, replica shard syncs segment files from primary. Thus, verifying doc count on all shard copies (specially replica) is sufficient. When segment replication is not working correctly, the replica wouldn't be able to copy files correctly and thus shouldn't have same documents which were ingested.
@dreamer-89 looks like there are no more comments for code itself, could you please re-run all the CI checks
@dreamer-89 are you still working on this PR?
@dreamer-89 are you still working on this PR?
Thanks @navneet1v @martin-gaievski for checking in. Please let me know if you have any open questions/concerns regarding this change. If not, lets move forward with PR approval and code merge.
[Edit]: I did force push due to conflict on rebase against main
@dreamer-89 the build is broken please fix the build.