Skip to content
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

Add basic integration test for segment replication #927

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dreamer-89
Copy link
Member

@dreamer-89 dreamer-89 commented Jun 5, 2023

Description

This PR adds an integration test to do sanity for segment replication.

Issues Resolved

#921

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • 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
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9e4251e) 84.92% compared to head (a364584) 85.10%.
Report is 1 commits behind head on main.

❗ 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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@martin-gaievski martin-gaievski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the breaking change? Seems it doesn't work with rolling upgrades, or that is work in progress?

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.

@dreamer-89
Copy link
Member Author

dreamer-89 commented Jun 6, 2023

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 opensearch-project/OpenSearch#3881

Added more details in #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.

@martin-gaievski
Copy link
Member

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

@navneet1v
Copy link
Collaborator

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

@dreamer-89
Copy link
Member Author

dreamer-89 commented Jan 31, 2024

@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

@navneet1v
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants