k-NN icon indicating copy to clipboard operation
k-NN copied to clipboard

Add basic integration test for segment replication

Open dreamer-89 opened this issue 2 years ago • 6 comments
trafficstars

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.

dreamer-89 avatar Jun 05 '23 23:06 dreamer-89

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.

codecov[bot] avatar Jun 06 '23 00:06 codecov[bot]

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 avatar Jun 06 '23 17:06 dreamer-89

@dreamer-89 looks like there are no more comments for code itself, could you please re-run all the CI checks

martin-gaievski avatar Jul 11 '23 18:07 martin-gaievski

@dreamer-89 are you still working on this PR?

navneet1v avatar Jan 31 '24 06:01 navneet1v

@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 avatar Jan 31 '24 19:01 dreamer-89

@dreamer-89 the build is broken please fix the build.

navneet1v avatar Feb 06 '24 18:02 navneet1v