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

Revert "Optimize remote state stale file deletion (#13131)" #13969

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

reta
Copy link
Collaborator

@reta reta commented Jun 4, 2024

This reverts commit b9befaa.

Description

Introduces new flaky tests #13968

Related Issues

Reverting #13131

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • API changes companion pull request created.
  • Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  • Public documentation issue/PR created

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.

@reta
Copy link
Collaborator Author

reta commented Jun 4, 2024

@andrross @dblock reverting due to new flaky tests, @sachinpkale @shiv0408 please resubmit when flakyness is understood and eliminated.

Copy link
Contributor

github-actions bot commented Jun 4, 2024

❌ Gradle check result for cdd54a1: 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?

@reta
Copy link
Collaborator Author

reta commented Jun 4, 2024

❌ Gradle check result for cdd54a1: FAILURE

#12743

@shiv0408
Copy link
Member

shiv0408 commented Jun 4, 2024

@reta rather than reverting the whole change, can we disable the test instead? I found the reason for flakiness and we can discuss how we can modify the test to accommodate that. Does necessarily need to revert the whole change.

Copy link
Contributor

github-actions bot commented Jun 4, 2024

❕ Gradle check result for cdd54a1: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Copy link

codecov bot commented Jun 4, 2024

Codecov Report

Attention: Patch coverage is 27.77778% with 91 lines in your changes missing coverage. Please review.

Project coverage is 71.71%. Comparing base (b15cb0c) to head (4bdac24).
Report is 342 commits behind head on main.

Files Patch % Lines
...arch/gateway/remote/RemoteClusterStateService.java 28.00% 90 Missing ⚠️
server/src/main/java/org/opensearch/node/Node.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #13969      +/-   ##
============================================
+ Coverage     71.42%   71.71%   +0.29%     
- Complexity    59978    61367    +1389     
============================================
  Files          4985     5065      +80     
  Lines        282275   288188    +5913     
  Branches      40946    41738     +792     
============================================
+ Hits         201603   206666    +5063     
- Misses        63999    64469     +470     
- Partials      16673    17053     +380     

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

@reta
Copy link
Collaborator Author

reta commented Jun 4, 2024

@reta rather than reverting the whole change, can we disable the test instead? I found the reason for flakiness and we can discuss how we can modify the test to accommodate that. Does necessarily need to revert the whole change.

@shiv0408 the nature of the failure is unknown - either impl is not stable or test is not stable, please spend more time on the change and resubmit it.

@shiv0408
Copy link
Member

shiv0408 commented Jun 4, 2024

Reverting this commit will just bring another flaky test #12798, which was removed as part of this.
If we can wait for a day to merge this, I would rather want to raise a fix for the test.

@reta
Copy link
Collaborator Author

reta commented Jun 4, 2024

Reverting this commit will just bring another flaky test #12798, which was removed as part of this.
If we can wait for a day to merge this, I would rather want to raise a fix for the test.

I will reopen the issue than, thanks for heads up

Copy link
Contributor

github-actions bot commented Jun 4, 2024

❌ Gradle check result for 4bdac24: 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?

@reta
Copy link
Collaborator Author

reta commented Jun 4, 2024

❌ Gradle check result for 4bdac24: FAILURE

#10314

@reta
Copy link
Collaborator Author

reta commented Jun 4, 2024

@andrross @kotwanikunal folks mind please reapproving (rebased)? thank you

Copy link
Contributor

github-actions bot commented Jun 4, 2024

❕ Gradle check result for 4bdac24: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@andrross andrross merged commit 97340ed into opensearch-project:main Jun 4, 2024
29 of 30 checks passed
akolarkunnu pushed a commit to akolarkunnu/OpenSearch that referenced this pull request Jun 5, 2024
gaobinlong pushed a commit to gaobinlong/OpenSearch that referenced this pull request Jun 5, 2024
LantaoJin pushed a commit to LantaoJin/OpenSearch that referenced this pull request Jun 6, 2024
parv0201 pushed a commit to parv0201/OpenSearch that referenced this pull request Jun 10, 2024
wdongyu pushed a commit to wdongyu/OpenSearch that referenced this pull request Aug 22, 2024
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.

4 participants