-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Add TEST MergeWithLowDiskSpaceIT testForceMergeIsBlockedThenUnblocked #130189
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
Add TEST MergeWithLowDiskSpaceIT testForceMergeIsBlockedThenUnblocked #130189
Conversation
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
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
I hope we can avoid the thread though.
); | ||
} | ||
// start force merging (which is blocking) on a separate thread | ||
Thread forceMergeThread = new Thread( |
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.
Could we avoid the .get()
call and use execute()
instead to avoid the thread? Assertions further down could then be about whether the future is done or not.
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.
Of course, thanks for the suggestion! (I've handled it as part of 3452dce)
long totalMergeCount = indicesStatsResponse.getIndices().get(indexName).getPrimaries().merge.getTotal(); | ||
assertThat(totalMergeCount, greaterThan(0L)); | ||
// force merge call returned | ||
assertFalse(forceMergeThread.isAlive()); |
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.
Could we instead wait for the force merge to complete first? If we go for a future, we can use future.get()
here (or safeGet). If not, we can use forcemergeThread.join
. That way I think we can avoid the assertBusy
, since all the things here should hopefully be up to date.
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.
Great suggestion too, thanks! (also handled as part of 3452dce)
…elastic#130189) This adds a new IT for when forced merges are blocked (and then unblocked) because of the insufficient disk space situation. This test was suggested elastic#127613 (review).
…elastic#130189) This adds a new IT for when forced merges are blocked (and then unblocked) because of the insufficient disk space situation. This test was suggested elastic#127613 (review).
…#130189) (#130288) This adds a new IT for when forced merges are blocked (and then unblocked) because of the insufficient disk space situation. This test was suggested #127613 (review).
…elastic#130189) This adds a new IT for when forced merges are blocked (and then unblocked) because of the insufficient disk space situation. This test was suggested elastic#127613 (review).
…blocked (#130189) (#130289) * Add TEST MergeWithLowDiskSpaceIT testForceMergeIsBlockedThenUnblocked (#130189) This adds a new IT for when forced merges are blocked (and then unblocked) because of the insufficient disk space situation. This test was suggested #127613 (review). * Add TEST MergeWithLowDiskSpaceIT testForceMergeIsBlockedThenUnblocked (#130189) This adds a new IT for when forced merges are blocked (and then unblocked) because of the insufficient disk space situation. This test was suggested #127613 (review). * Telemetry metrics don't exist in 8.19
…locked (#130189) (#130287) * Add TEST MergeWithLowDiskSpaceIT testForceMergeIsBlockedThenUnblocked (#130189) This adds a new IT for when forced merges are blocked (and then unblocked) because of the insufficient disk space situation. This test was suggested #127613 (review). * merge telemetry metrics don't exist in 9.0 * Fix MergeWithLowDiskSpaceIT post telemetry plugin removal * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <[email protected]>
This adds a new IT for when forced merges are blocked (and then unblocked) because of the insufficient disk space situation.
This test was suggested #127613 (review).