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

Set NoMergePolicy for codec tests #754

Merged
merged 3 commits into from
Feb 14, 2023

Conversation

jmazanec15
Copy link
Member

@jmazanec15 jmazanec15 commented Feb 8, 2023

Description

Sets NoMerge as merge policy for codec tests. This allows us to reliably predict how many segments will be created.

This fixes flaky test case that caused 3.0 build to fail

Issues Resolved

#750

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.

Sets NoMerge as merge policy for codec tests. This allows us to reliably
predict how many segments will be created.

Signed-off-by: John Mazanec <[email protected]>
@jmazanec15 jmazanec15 added Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. backport 2.x backport 2.5 labels Feb 8, 2023
@jmazanec15 jmazanec15 requested a review from a team February 8, 2023 22:22
@martin-gaievski
Copy link
Member

What is the policy that is applied now, before this change?

@@ -317,6 +320,7 @@ public void testKnnVectorIndex(
IndexWriterConfig iwc = newIndexWriterConfig();
iwc.setMergeScheduler(new SerialMergeScheduler());
iwc.setCodec(codec);
iwc.setMergePolicy(NoMergePolicy.INSTANCE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be nice to have a comment explaining why we need this policy.
Shouldn't we modify the test itself so that it won't fail even without specifying this policy?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add a comment.

For testBuildFromModelTemplate and testKnnVectorIndex I dont think they are needed so I will remove.

For testMultiFieldsKnnIndex, in

, we are trying to guarantee that for each field, there is one hnsw file getting created. As an alternative to setting NoMergePolicy, I could refactor this check for at least one of each file. What do you think for that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

More specific test with less chance of missing fault is better. In that sense, using NoMergePolicy is better I guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. I updated and added a comment.

@jmazanec15
Copy link
Member Author

@martin-gaievski TieredMergePolicy is used.

@jmazanec15 jmazanec15 requested a review from heemin32 February 13, 2023 18:20
@codecov-commenter
Copy link

codecov-commenter commented Feb 13, 2023

Codecov Report

Merging #754 (2da421e) into main (4de9f83) will decrease coverage by 0.33%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main     #754      +/-   ##
============================================
- Coverage     85.10%   84.78%   -0.33%     
+ Complexity     1080     1072       -8     
============================================
  Files           151      151              
  Lines          4370     4370              
  Branches        390      390              
============================================
- Hits           3719     3705      -14     
- Misses          473      485      +12     
- Partials        178      180       +2     
Impacted Files Coverage Δ
...va/org/opensearch/knn/index/KNNCircuitBreaker.java 60.00% <0.00%> (-20.00%) ⬇️
...earch/knn/index/codec/KNN910Codec/KNN910Codec.java 87.50% <0.00%> (-12.50%) ⬇️
...earch/knn/index/codec/KNN940Codec/KNN940Codec.java 90.00% <0.00%> (-10.00%) ⬇️
...earch/knn/index/codec/KNN920Codec/KNN920Codec.java 81.81% <0.00%> (-9.10%) ⬇️
...ain/java/org/opensearch/knn/index/KNNSettings.java 84.37% <0.00%> (-2.35%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: John Mazanec <[email protected]>
@jmazanec15 jmazanec15 merged commit b8f2deb into opensearch-project:main Feb 14, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 14, 2023
Sets NoMerge as merge policy for codec test. This allows us to reliably
predict how many segments will be created.

Signed-off-by: John Mazanec <[email protected]>
(cherry picked from commit b8f2deb)
opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 14, 2023
Sets NoMerge as merge policy for codec test. This allows us to reliably
predict how many segments will be created.

Signed-off-by: John Mazanec <[email protected]>
(cherry picked from commit b8f2deb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport 2.5 Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants