Use timely fabric to forward ComputeCommands #14485
Conversation
antiguru
left a comment
antiguru
left a comment
There was a problem hiding this comment.
This looks mostly fine. I think there are some cases that can be simplified, and we want to preserve the behavior that commands are acted on in batches.
src/compute/src/server.rs
Outdated
|
|
||
| move |output| { | ||
| //println!("w{:?} Calling non-blocking recv", &idx); | ||
| if idx == 0 { |
There was a problem hiding this comment.
This is a suspenseful moment, because
- We don't drop
capifidx != 0 - What happens to commands received by a different process than process 0? It seems we would enqueue them indefinitely with this change.
There was a problem hiding this comment.
I changed this now to panic if idx != 0 receives a message.
src/compute/src/server.rs
Outdated
| fn recv<A: Allocate>(&mut self, worker: &mut Worker<A>) -> Result<ComputeCommand, RecvError> { | ||
| // This takes the worker, so it can step timely while no result | ||
| // is available. |
There was a problem hiding this comment.
I wonder if there's an alternative design where we broadcast the initialization commands to all workers and only unicast once the initialization is done? This might have the downside of added complexity and making it harder to recover from errors during initialization.
There was a problem hiding this comment.
I think that's a possibility. Also regarding #14379 . It might make the cloud deployment easier if computed's start as "empty shell" and receive their configuration in the CreateInstance. If we do this, broadcasting the CreateInstance is a requirement.
|
Thanks @antiguru, I first figure out what goes wrong with the tests then address your comments |
lluki
force-pushed
the
fix_13603_22
branch
3 times, most recently
from
51f9b98 to
2707c9e
Compare
|
I'm thinking of these lines (server.rs:382). I'm pretty confident we can replaced them with a simple |
teskje
left a comment
teskje
left a comment
There was a problem hiding this comment.
I had a couple small comments. I think I need to spend a Friday learning about the computed server before I'm able to understand the changes there though!
|
I'm not sure about removing the |
We can't guarantee of course that we don't introduce one in the future. But as long as there are channels acting as barriers during setup, it shouldn't happen again. But as a safeguard we can also just keep it. |
antiguru
left a comment
antiguru
left a comment
There was a problem hiding this comment.
I think this looks good. Thanks!
| let mut r = vec![None; self.parts]; | ||
| r[0] = Some(command); | ||
| r |
There was a problem hiding this comment.
I think I would have use iterator magic (once(command).chain(repeat(None, self.parts-1)) or something like this, but your version does the same job.
…erializeInc#14189)" This reverts commit 7bcca12.
Partitioned client send commands only to first part. The timely worker at index 0 then uses timely to disseminate the commands to the remaining workers.
The channel for sending back the rx_activator now acts as a barrier, which fixes the same bug.
3 participants
This PR implements option 2) of MaterializeInc/database-issues#3881 : Environmentd sends command only to the first process, which then uses timely internally to disseminate the command. Responses are still sent over the network from each process.
Fixes MaterializeInc/database-issues#3881
Motivation
Tips for reviewer
Checklist
This PR has adequate test coverage / QA involvement has been duly considered.
This PR evolves an existing
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way) and therefore is tagged with aT-protobuflabel.This PR includes the following user-facing behavior changes: