Move scheduling of delayed ServiceInvocations from caller PP to callee PP. #1306
Conversation
slinkydeveloper
force-pushed
the
issues/1292
branch
from
8997bb2
to
459055c
Compare
There was a problem hiding this comment.
Thanks for creating this PR @slinkydeveloper. The changes look good to me modulo the additional round of replication which we should try to avoid. Once this has been fixed, +1 for merging :-)
crates/worker/src/partition/services/non_deterministic/idempotent_invoker.rs
Outdated
Show resolved
Hide resolved
| // Remove the execution time from the service invocation request | ||
| service_invocation.execution_time = None; | ||
|
|
||
| // TODO(optimization) we could skip the outbox here and re-propose this to self. |
There was a problem hiding this comment.
I think it would make sense to do this "optimization". Otherwise, we are adding an additional replication round for delayed calls which is not strictly necessary but a regression compared to before. I think we don't need to re-propose to ourselves if the service_invocation is targeted to ourselves. Instead, we can directly call handle_invoke and be done with it.
There was a problem hiding this comment.
Technically, this (skipping the outbox for self-targeted messages) could also apply to immediate background calls or calls.
There was a problem hiding this comment.
could also apply to immediate background calls or calls.
Immediate you mean execution_time is past now?
There was a problem hiding this comment.
With immediate I meant non-delayed invocations (invoke_time == 0).
There was a problem hiding this comment.
Isn't this PR already covering it?
There was a problem hiding this comment.
When getting a journal entry for a call/background call we go through the outbox because we don't know if the current PP owns the invocation or not, this PR did not change that. Perhaps an optimization is worth implementing anyway, indipendently from this PR?
|
@tillrohrmann I asked your review again as I simplified the storage format + optimized the timer handling, where now we don't go through the outbox anymore. |
There was a problem hiding this comment.
Thanks for updating this PR @slinkydeveloper. I left two comments which we should resolve before merging.
crates/worker/src/partition/state_machine/command_interpreter/mod.rs
Outdated
Show resolved
Hide resolved
…iceId for Invoker timers now.
slinkydeveloper
force-pushed
the
issues/1292
branch
from
9255e1b
to
1c8b505
Compare
Fix #1292