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
Remove Blocking on GENERIC Pool in GET Snapshots #69101
Conversation
This aligns the way get snapshots works in `7.x` with how it works in `master` exactly (except for masters ability to load snapshots across repos) and thus fixes the deadlock in elastic#69099. Closes elastic#69099
Pinging @elastic/es-distributed (Team:Distributed) |
Jenkins run elasticsearch-ci/1 (snapshot test failure but unrelated and known to the features team) |
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 opened #69108 to remind us to do something better with the new response format in 8.0 when we can.
@@ -170,16 +181,17 @@ protected void masterOperation(final GetSnapshotsRequest request, final ClusterS | |||
* if false, they will throw an error | |||
* @return list of snapshots | |||
*/ | |||
private List<SnapshotInfo> snapshots(@Nullable SnapshotsInProgress snapshotsInProgress, String repositoryName, | |||
private List<SnapshotInfo> snapshots(SnapshotsInProgress snapshotsInProgress, String repositoryName, | |||
List<SnapshotId> snapshotIds, boolean ignoreUnavailable) { |
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.
Nit: spurious whitespace deviation from master :)
List<SnapshotId> snapshotIds, boolean ignoreUnavailable) { | |
List<SnapshotId> snapshotIds, boolean ignoreUnavailable) { |
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.
The version here is actually nicer/more correct :D that's why I deviated. I'd rather fix this in master next time a change to the file is made there :)
Thanks David! |
This aligns the way get snapshots works in `7.x` with how it works in `master` exactly (except for masters ability to load snapshots across repos) and thus fixes the deadlock in elastic#69099. Closes elastic#69099
This aligns the way get snapshots works in
7.x
with how it worksin
master
exactly (except for masters ability to load snapshots across repos)and thus fixes the deadlock in #69099.
No need to review the logic in detail here, it's the exact same logic as in master now so it probably rather makes sense comparing with master than looking at this in isolation.
Closes #69099