Introduce ControlProcessors command #1698
Conversation
tillrohrmann
force-pushed
the
issues/1695
branch
from
15d7756
to
6a79b43
Compare
tillrohrmann
changed the title
| async fn run_for_leader(&self, leader_epoch: LeaderEpoch) -> Result<(), ShutdownError> { | ||
| self.control_tx | ||
| .send(PartitionProcessorControlCommand::RunForLeader(leader_epoch)) | ||
| .await |
There was a problem hiding this comment.
I'm not sure if we should await in those cases. We don't want to block the control loop if a single processor is too busy, right?
Perhaps we can set a timeout at the call-site or switch to a try_send?
There was a problem hiding this comment.
Yes, this is indeed a problem. I am also not really happy with it right now. Switching it to try_send could be a first band-aid. I'll change it that way and open an issue to revisit the PartitionProcessor because there are also some other places where we await.
| name_cache: BTreeMap<PartitionId, &'static str>, | ||
|
|
||
| metadata: Metadata, | ||
| metadata_store_client: MetadataStoreClient, | ||
| partition_store_manager: PartitionStoreManager, | ||
| attach_router: RpcRouter<AttachRequest, Networking>, | ||
| incoming_get_state: BoxStream<'static, MessageEnvelope<GetProcessorsState>>, | ||
| incoming_get_state: | ||
| Pin<Box<dyn Stream<Item = MessageEnvelope<GetProcessorsState>> + Send + Sync + 'static>>, |
There was a problem hiding this comment.
Curious, why did we need to remove BoxStream stream?
There was a problem hiding this comment.
I needed to add the Sync trait to the Stream trait object. That's why I had to spell things out more verbosely :-(
| for control_processor in control_processors.commands { | ||
| self.on_control_processor(control_processor, &partition_table) | ||
| .await?; | ||
| } |
There was a problem hiding this comment.
I'm a little concerned about critical control loops with inner awaits. Best if we can execute concurrently but it might be too much of a hassle given how everything is structured. Alternatives could be to do fail early on potential blocking points, or add a timeout to limit waiting time upper bound.
There was a problem hiding this comment.
Yes, this is not good. As a temporary band-aid, I will replace the sends with try_send as you've suggested. It still does not make control loop completely await-free. I will take a look at this in a separate step.
This commit introduces the ControlProcessors command which allows an external entity to instruct the PartitionProcessorManager to start and stop PartitionProcessors. This fixes restatedev#1695.
tillrohrmann
force-pushed
the
issues/1695
branch
from
6a79b43
to
dbae1a5
Compare
This commit introduces the ControlProcessors command which allows an
external entity to instruct the PartitionProcessorManager to start
and stop PartitionProcessors.
This fixes #1695.
This PR is based on #1694.