Make Repository.getRepositoryData an Async API #49299
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This API call in most implementations is fairly IO heavy and slow so it is more natural to be async in the first place. Concretely though, this change is a prerequisite of elastic#49060 since determining the repository generation from the cluster state introduces situations where this call would have to wait for other operations to finish. Doing so in a blocking manner would break `SnapshotResiliencyTests` and waste a thread. Also, this sets up the possibility to in the future make use of async IO where provided by the underlying Repository implementation. In a follow-up `SnapshotsService#getRepositoryData` will be made async as well (did not do it here, since it's another huge change to do so). Note: This change for now does not alter the threading behavior in any way with the exception of blocking a GENERIC thread in `SnapshotsService#getRepositoryData` (this should be fine since this API is only used by status APIs) and is purely mechanical otherwise.
original-brownbear
added
>non-issue
:Distributed/Snapshot/Restore
|
Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore) |
|
Thanks Yannick! |
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Nov 19, 2019
This API call in most implementations is fairly IO heavy and slow so it is more natural to be async in the first place. Concretely though, this change is a prerequisite of elastic#49060 since determining the repository generation from the cluster state introduces situations where this call would have to wait for other operations to finish. Doing so in a blocking manner would break `SnapshotResiliencyTests` and waste a thread. Also, this sets up the possibility to in the future make use of async IO where provided by the underlying Repository implementation. In a follow-up `SnapshotsService#getRepositoryData` will be made async as well (did not do it here, since it's another huge change to do so). Note: This change for now does not alter the threading behaviour in any way (since `Repository#getRepositoryData` isn't forking) and is purely mechanical.
original-brownbear
added a commit
that referenced
this pull request
Nov 19, 2019
This API call in most implementations is fairly IO heavy and slow so it is more natural to be async in the first place. Concretely though, this change is a prerequisite of #49060 since determining the repository generation from the cluster state introduces situations where this call would have to wait for other operations to finish. Doing so in a blocking manner would break `SnapshotResiliencyTests` and waste a thread. Also, this sets up the possibility to in the future make use of async IO where provided by the underlying Repository implementation. In a follow-up `SnapshotsService#getRepositoryData` will be made async as well (did not do it here, since it's another huge change to do so). Note: This change for now does not alter the threading behaviour in any way (since `Repository#getRepositoryData` isn't forking) and is purely mechanical.
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Nov 19, 2019
Follow up to elastic#49299 removing the blocking step for the snapshot status APIs as well.
original-brownbear
added a commit
that referenced
this pull request
Nov 20, 2019
* Make SnapshotsService#getRepositoryData Async Follow up to #49299 removing the blocking step for the snapshot status APIs as well.
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Nov 20, 2019
* Make SnapshotsService#getRepositoryData Async Follow up to elastic#49299 removing the blocking step for the snapshot status APIs as well.
original-brownbear
added a commit
that referenced
this pull request
Nov 20, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This API call in most implementations is fairly IO heavy and slow
so it is more natural to be async in the first place.
Concretely though, this change is a prerequisite of #49060 since
determining the repository generation from the cluster state
introduces situations where this call would have to wait for other
operations to finish. Doing so in a blocking manner would break
SnapshotResiliencyTestsand waste a thread.Also, this sets up the possibility to in the future make use of async IO
where provided by the underlying Repository implementation.
In a follow-up
SnapshotsService#getRepositoryDatawill be made asyncas well (did not do it here, since it's another huge change to do so).
Note: This change for now does not alter the threading behaviour in any way (since
Repository#getRepositoryDataisn't forking) and is purely mechanical.Note to reviewers: I did my best to not add new line-breaks and such so this should be easy to review using
?w=1(only +125 - 80 lines in that case) hopefully :)