persist: start replacing CmdResponse with Future #7630
Conversation
src/persist/src/indexed/runtime.rs
Outdated
| /// A receiver for the `Result<T, Error>` response of an asynchronous command. | ||
| // | ||
| // TODO: Does this even get us anything now that we have [Future]? The only |
There was a problem hiding this comment.
I'm leaning toward doing this TODO in this PR. Worst case, we can pull it back out of git history.
There was a problem hiding this comment.
This seems good to me! Certainly the external interface seems much nicer and seems like we got to move a lot of complexity out of the persist crate.
My mental model around futures is not the greatest so I guess my only question is: how often will this get poll-ed? The underlying future executor won't have any visibility into whats going on right, it just knows that theres a channel still waiting for a value?
I'm sure that's all fine, just trying to build a more fine grained understanding of what's going on
| @@ -27,6 +27,8 @@ pub mod mem; | |||
| #[cfg(test)] | |||
| pub mod nemesis; | |||
| pub mod operators; | |||
| // TODO: Rename this to future once we rename indexed::future to unsealed. | |||
| pub mod pfuture; | |||
| @@ -3093,7 +3098,13 @@ impl Coordinator { | |||
| .collect(); | |||
| // Persistence of system table inserts is best effort, so throw | |||
| // away the response and ignore any errors. | |||
| persist.write_handle.write(&updates, CmdResponse::Ignore); | |||
| // | |||
| // NB: Keep this method call outside the tokio::spawn. We're | |||
There was a problem hiding this comment.
this is minor but if persistence of table is best effort (as per the comment above) then does it matter that this write happens in order?
There was a problem hiding this comment.
It might get reordering after a seal that turns it into an error. I'd call that not "best" effort, but ultimately no it doesn't really matter here. Added a note to the comment.
There was a problem hiding this comment.
Thanks for the reviews everyone!
My mental model around futures is not the greatest so I guess my only question is: how often will this get poll-ed?
I don't have a deep understanding of future executors, but my mental model is that (while the future contract allows unlimited polling) in practice an executor would poll the future once, installing a Waker, and then let it sit until the Waker is activated. Most futures call wake whenever they can make progress (finish some blocking operation), which causes the executor to call poll again, allowing them to make incremental progress until the next blocking bit, at which point they again return Pending from poll, starting the whole process over again. This one is a little different in that it is driven to completion by the runtime thread and so only calls wake once when the final value is ready. (However, anything that happens after the .into_future() call, like the various maps that transform errors in this PR, follows the normal rules.)
The underlying future executor won't have any visibility into whats going on right, it just knows that theres a channel still waiting for a value?
Indeed it does not have any visibility into what's going on. Just the receiving end of a oneshot::channel.
| @@ -3093,7 +3098,13 @@ impl Coordinator { | |||
| .collect(); | |||
| // Persistence of system table inserts is best effort, so throw | |||
| // away the response and ignore any errors. | |||
| persist.write_handle.write(&updates, CmdResponse::Ignore); | |||
| // | |||
| // NB: Keep this method call outside the tokio::spawn. We're | |||
There was a problem hiding this comment.
It might get reordering after a seal that turns it into an error. I'd call that not "best" effort, but ultimately no it doesn't really matter here. Added a note to the comment.
The original idea behind CmdResponse (plumbing a channel all the way from persist to pgwire) hasn't ended up working out. It turns out that callers of write and seal just want a std::future::Future, so give them one. The persist Future is powered by a oneshot::channel(), but hide this implementation detail. Bonus: This much more elegantly handles channel errors than what we had before.
The original idea behind CmdResponse (plumbing a channel all the way
from persist to pgwire) hasn't ended up working out. It turns out that
callers of write and seal just want a std::future::Future, so give them
one.
The persist Future is powered by a oneshot::channel(), but hide this
implementation detail. Bonus: This much more elegantly handles channel
errors than what we had before.