Let PartitionProcessors failures propagate to the PartitionProcessorManager #2214
Conversation
muhamadazmy
force-pushed
the
pr2214
branch
4 times, most recently
from
3d033bd
to
cf35633
Compare
|
This change prevents a failure of a PP from bringing down the entire runtime, instead the error is propagated to the PPM and the status of the PP is basically popped out from the inner map. This will then show up on the CC next status pull and can then decide to take an action. |
muhamadazmy
force-pushed
the
pr2214
branch
2 times, most recently
from
a4cd855
to
e09cb67
Compare
There was a problem hiding this comment.
Thanks for this PR @muhamadazmy. The changes look good to me. I had one comment regarding the clean shut down case where a Stopped event could also be helpful. +1 for merging after resolving the comments.
| @@ -648,6 +652,14 @@ impl<T: TransportConnect> PartitionProcessorManager<T> { | |||
| error!(%partition_id, error=%err, "Starting partition processor failed"); | |||
| self.running_partition_processors.remove(&partition_id); | |||
| } | |||
| ProcessorEvent::Stopped(err) => { | |||
| error!(%partition_id, error=%err, "Partition processor exited"); | |||
There was a problem hiding this comment.
Maybe warn because the system is not yet broken (hopefully). warn!(%partition_id, "Partition processor exited unexpectedly: {err}");
| let _ = events | ||
| .send(ManagerEvent { | ||
| partition_id, | ||
| event: ProcessorEvent::Stopped(err), |
There was a problem hiding this comment.
i would probably also return a Stopped event if the partition processor exited w/o an error. That way the receiver can also clean up its data structures.
There was a problem hiding this comment.
Yes, indeed. Good catch!
muhamadazmy
force-pushed
the
pr2214
branch
2 times, most recently
from
c63d90b
to
67aaa83
Compare
…anager Summary: When PP crashes (returns an error) we should update it's status instead of panicing Fixes restatedev#2147
Let PartitionProcessors failures propagate to the PartitionProcessorManager
Summary:
When PP crashes (returns an error) we should update it's status instead
of panicing
Fixes #2147
Stack created with Sapling. Best reviewed with ReviewStack.