-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Clean up SearchableSnapshotActionIT
#134305
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
Clean up SearchableSnapshotActionIT
#134305
Conversation
As a follow-up of elastic#133954, this class could use a clean up in deduplicating code, replacing some `assertBusy`s with `awaitIndexExists`, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the SearchableSnapshotActionIT
test class to reduce code duplication, improve maintainability, and replace repetitive assertBusy
calls with more specific helper methods.
- Replaces manual index existence checks and
assertBusy
withawaitIndexExists
andawaitIndexDoesNotExist
- Consolidates duplicate snapshot retrieval logic into a reusable
getSnapshots()
method - Replaces custom step key checking with
TimeSeriesRestDriver.awaitStepKey()
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
SearchableSnapshotActionIT.java | Removes duplicate code patterns, replaces assertBusy with helper methods, removes unused imports and suppressions |
TimeSeriesRestDriver.java | Adds awaitStepKey method and makes getStepKey public to support refactoring |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...de/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java
Show resolved
Hide resolved
Pinging @elastic/es-data-management (Team:Data Management) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though I notice we're changing a lot of timeouts to be shorter. Do you have any concerns with that, or do we think things have sped up to where they shouldn't cause failures?
assertBusy(() -> assertFalse(indexExists(backingIndexName)), 60, TimeUnit.SECONDS); | ||
assertBusy(() -> assertFalse(indexExists(restoredIndexName)), 60, TimeUnit.SECONDS); | ||
awaitIndexDoesNotExist(backingIndexName); | ||
awaitIndexDoesNotExist(restoredIndexName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're switching these here from a 60 second timeout to a 10 second timeout, do you have any concerns about this failing in some cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I could tell by looking at the history of this class, most timeouts looked like they were simply copied over from other places. And these 60-second timeouts are simply excessive; there is no way ILM needs a whole minute to get to that stage. I decided to clear out all custom timeouts to have a clean slate. I've been running this class on repeat for more than a day on 3 VMs. While that doesn't prove that no failures will occur on our CI VMs, I feel confident to merge this PR as-is. If failures do come up, I will take it upon myself to have a look at them. Although I'd probably first look at changing the setup/order of the test before resorting to simply increasing timeouts.
As a follow-up of elastic#133954, this class could use a clean up in deduplicating code, replacing some `assertBusy`s with `awaitIndexExists`, and more.
As a follow-up of #133954, this class could use a clean up in deduplicating code, replacing some
assertBusy
s withawaitIndexExists
, and more.