ct: split persist writes into a separate operator #30147
Conversation
This does two things: - Makes it straightforward to support readonly mode. - Sets up swapping in storage's multi-worker persist_sink, once it supports truncating on CaA failure and is less entagled with storage. It feels a little silly to write a throwaway single-worker persist_sink, but it's nice that the multi-worker one should be a drop-in replacement.
| let (to_append, button0) = | ||
| continual_task_sink(&name, to_append, append_times, as_of, write_handle()); |
There was a problem hiding this comment.
Nit: Now that this operator is not a sink anymore, can we call it something else?
There was a problem hiding this comment.
I had the same thought, but didn't think of anything good for the name. Any ideas?
There was a problem hiding this comment.
No, I'm drawing a blank as well :/
| if let Some(new_upper) = new_upper.as_option() { | ||
| cap.downgrade(new_upper); | ||
| }; |
There was a problem hiding this comment.
Why don't we need to drop the capability when new_upper is the empty frontier? Are we guaranteed to take the "Inputs exhausted, shutting down" branch next in that case?
| @@ -394,11 +395,10 @@ fn continual_task_sink<G: Scope<Timestamp = Timestamp>>( | |||
| append_times: Collection<G, (), Diff>, | |||
| as_of: Antichain<Timestamp>, | |||
| write_handle: impl Future<Output = WriteHandle<SourceData, (), Timestamp, Diff>> + Send + 'static, | |||
| start_signal: StartSignal, | |||
There was a problem hiding this comment.
Nit: There is a outdated comment a couple lines below that talks about "The start_signal below...", which is gone now.
|
(I might hold off on merging this until @def- finishes the first full round of CT testing. It doesn't seem super pressing and I'm a little worried that it'll introduce some silly small bug and add another hiccup to the timeline.) |
|
Cleaning up old PRs |
This does two things:
It feels a little silly to write a throwaway single-worker persist_sink, but it's nice that the multi-worker one should be a drop-in replacement.
Touches https://github.com/MaterializeInc/database-issues/issues/8427
Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.