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

Fix IndicesRequestCacheIT flaky tests #16276

Merged
merged 2 commits into from
Oct 12, 2024

Conversation

sgup432
Copy link
Contributor

@sgup432 sgup432 commented Oct 10, 2024

Description

This makes an attempt to fix two flaky test in IndicesRequestCacheIT mentioned here

I was not able to reproduce this even when running these tests for more than 500 iterations. So pretty rare. But the fix is based on a pretty good assumption that it will work. Also considering experience with fixing such similar tests successfully in the past.
Details below:

  • testCacheWithFilteredAlias: This fails at a point when it expects cache hits but gets none(because items got invalidated). It gets invalidated because of background refreshes/merges which was also observed in rest of the tests in same file, and those don't report any issues now after the fix. So the fix is to disable refresh/merge and should work.
  • testTimedOutQuery: This tests tries to deliberately time out the query to test cache functionality and validate. Here we create a deliberate time out of 500ms(by Thread.sleep()) at a certain step(query.createWeight()) and set a timeout of 0ms. But 0ms is pretty aggressive and sometimes query times out before even reaching above step. For the fix, we increase the timeout to 5ms, this logic is the same which is being used in SearchTimeoutIT which works fine.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • [ ] Public documentation issue/PR created, if applicable.

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.

@sgup432 sgup432 added skip-changelog backport 2.x Backport to 2.x branch labels Oct 10, 2024
@sgup432 sgup432 changed the title Fix IndicesRequestCacheIt flaky tests Fix IndicesRequestCacheIT flaky tests Oct 10, 2024
Copy link
Contributor

❌ Gradle check result for 10c3dd2: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

LGTM as an attempt to fix. How are we tracking the follow-up (does this work? If not should we revert?)

@jainankitk
Copy link
Collaborator

@sgup432 - Seems you're missing the lucene version upgrade changes from main:

[Test Result](https://build.ci.opensearch.org/job/gradle-check/49251/testReport/) (1 failure / ±0)

    [org.opensearch.qa.verify_version_constants.VerifyVersionConstantsIT.testLuceneVersionConstant](https://build.ci.opensearch.org/job/gradle-check/49251/testReport/junit/org.opensearch.qa.verify_version_constants/VerifyVersionConstantsIT/testLuceneVersionConstant/)

@sgup432
Copy link
Contributor Author

sgup432 commented Oct 11, 2024

@dbwiddis

LGTM as an attempt to fix. How are we tracking the follow-up (does this work? If not should we revert?)

We will be tracking the subsequent failed tests as part of #14288. If we don't see any cases where above tests fail, I think we should be good. Otherwise need to revisit and try again.

Copy link
Contributor

✅ Gradle check result for 3c9421b: SUCCESS

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.99%. Comparing base (691f725) to head (3c9421b).
Report is 7 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #16276      +/-   ##
============================================
- Coverage     72.06%   71.99%   -0.07%     
+ Complexity    64822    64789      -33     
============================================
  Files          5308     5308              
  Lines        302574   302565       -9     
  Branches      43710    43710              
============================================
- Hits         218048   217845     -203     
- Misses        66648    66841     +193     
- Partials      17878    17879       +1     

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

@dbwiddis dbwiddis merged commit 20536ee into opensearch-project:main Oct 12, 2024
39 of 40 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 12, 2024
Signed-off-by: Sagar Upadhyaya <[email protected]>
(cherry picked from commit 20536ee)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
dbwiddis pushed a commit that referenced this pull request Oct 12, 2024
(cherry picked from commit 20536ee)

Signed-off-by: Sagar Upadhyaya <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
dk2k pushed a commit to dk2k/OpenSearch that referenced this pull request Oct 16, 2024
dk2k pushed a commit to dk2k/OpenSearch that referenced this pull request Oct 17, 2024
dk2k pushed a commit to dk2k/OpenSearch that referenced this pull request Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants