Conversation
|
Pinging @elastic/es-distributed |
DaveCTurner
left a comment
There was a problem hiding this comment.
Seems like a nice approach. As in #37076 I'm unclear about whether we still need CancellableThreads here, but that doesn't need resolving right now.
|
I think we should start step by step making things async. I don't necessarily understand why we depend this on the work on CompleteableFuture. I don't think we should use futures unless absolutely necessary and should use callbacks / listeners instead, consistently across the codebase. There are many things that I personally dislike about futures, the main issue here is that you need to be explicit on the return value and it's not obvious that the method is async. I would really like to prevent this and rather get rid of futures where we can. |
Yes, that's my plan if we agree to make recovery async.
Sure, I will use listeners for this. With CompleteableFuture, the flow of the async code is somehow similar to the sequential flow. That's the only reason I prefer CompleteableFuture over callback. |
Today we create a global instance of RecoveryResponse then mutate it when executing each recovery step. This is okay for the current sequential recovery flow but not suitable for an asynchronous recovery which we are targeting. With this commit, we return the result of each step separately, then construct a RecoveryResponse at the end. Relates #37174
Today we create a global instance of RecoveryResponse then mutate it when executing each recovery step. This is okay for the current sequential recovery flow but not suitable for an asynchronous recovery which we are targeting. With this commit, we return the result of each step separately, then construct a RecoveryResponse at the end. Relates #37174
I made this draft, so we have something to evaluate the async option. Since this PR is on top of #32512, please check out and review it locally or have a look at the last commit. If we agree to proceed, I will break down this into smaller PRs.