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

CardinalityIT/NestedIT test failures with concurrent search enabled and AssertingCodec #8687

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

sohami
Copy link
Collaborator

@sohami sohami commented Jul 13, 2023

Description

Backport of #8303

Related Issues

Resolves #8095

Check List

  • New functionality includes testing.
    • All tests pass
  • [] New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

…nd AssertingCodec (opensearch-project#8303)

* CardinalityIT/NestedIT test failures with concurrent search enabled and AssertingCodec
The tests were failing because during the concurrent segment search for each slice the
codec producers for the leafs were initialized by the slice thread. Later in reduce phase,
the post collection happens over those codec producers on the search thread.
With AssertingCodec it verifies that all access is done by the same thread causing the failures

Signed-off-by: Sorabh Hamirwasia <sohami.apache@gmail.com>

* Address review comments

Signed-off-by: Sorabh Hamirwasia <sohami.apache@gmail.com>

---------

Signed-off-by: Sorabh Hamirwasia <sohami.apache@gmail.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testPrimaryStopped_ReplicaPromoted
      1 org.opensearch.indices.replication.SegmentReplicationRelocationIT.testPrimaryRelocation

@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #8687 (6355800) into 2.x (90d1da2) will decrease coverage by 0.02%.
The diff coverage is 67.34%.

@@             Coverage Diff              @@
##                2.x    #8687      +/-   ##
============================================
- Coverage     70.61%   70.60%   -0.02%     
- Complexity    57254    57308      +54     
============================================
  Files          4756     4757       +1     
  Lines        271489   271521      +32     
  Branches      40019    40025       +6     
============================================
- Hits         191710   191701       -9     
- Misses        63340    63394      +54     
+ Partials      16439    16426      -13     
Impacted Files Coverage Δ
...nsearch/search/internal/FilteredSearchContext.java 8.92% <0.00%> (-0.25%) ⬇️
...arch/aggregations/DefaultAggregationProcessor.java 13.95% <50.00%> (+1.75%) ⬆️
.../search/aggregations/BucketCollectorProcessor.java 65.62% <65.62%> (ø)
.../org/opensearch/search/internal/SearchContext.java 38.00% <66.66%> (+1.82%) ⬆️
...va/org/opensearch/search/DefaultSearchContext.java 78.90% <100.00%> (+0.33%) ⬆️
...arch/aggregations/AggregationCollectorManager.java 83.33% <100.00%> (+10.00%) ⬆️
...h/aggregations/ConcurrentAggregationProcessor.java 81.25% <100.00%> (+1.25%) ⬆️
...ensearch/search/internal/ContextIndexSearcher.java 68.23% <100.00%> (+0.18%) ⬆️

... and 429 files with indirect coverage changes

@sohami
Copy link
Collaborator Author

sohami commented Jul 13, 2023

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testPrimaryStopped_ReplicaPromoted
      1 org.opensearch.indices.replication.SegmentReplicationRelocationIT.testPrimaryRelocation

unrelated failures.

@kotwanikunal
Copy link
Member

@sohami / @reta Code coverage does not meet the threshold. I think we need more UTs.

@sohami
Copy link
Collaborator Author

sohami commented Jul 13, 2023

@sohami / @reta Code coverage does not meet the threshold. I think we need more UTs.

@kotwanikunal The change is fixing the IT test listed in the title so the coverage is there with ITs and it will also execute for each of the aggregation search path which has extensive coverage

@kotwanikunal
Copy link
Member

@sohami / @reta Code coverage does not meet the threshold. I think we need more UTs.

@kotwanikunal The change is fixing the IT test listed in the title so the coverage is there with ITs and it will also execute for each of the aggregation search path which has extensive coverage

Ahh. Never realized ITs were not only not covered in codecov but are also considered as code. 💯

@kotwanikunal kotwanikunal merged commit 79b42b2 into opensearch-project:2.x Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants