Introduce TimerKind to support different types of timers #1480
Conversation
crates/wal-protocol/src/timer.rs
Outdated
| timer_key: TimerKey::new_invocation( | ||
| wake_up_time.as_u64(), | ||
| invocation_id.invocation_uuid(), | ||
| ), |
There was a problem hiding this comment.
I think this should be another timer kind, specific for the action of cleaning invocation status. Imagine we'll be able to support later cleaning up at separate times the invocation status and the idempotency key.
There was a problem hiding this comment.
What you are describing should already be supported. The one thing that is not supported is to have multiple actions scheduled at the same time for a single invocation because they would have the same key. Currently, we don't have this requirement. Once we have it, then we can introduce a new TimerKind for the concurrent action.
Maybe I should have called TimerKind TimerScope instead?
There was a problem hiding this comment.
I guess the argument one could make is that having a TimerKind per Timer would establish a one to one mapping between these two types.
There was a problem hiding this comment.
I guess the argument one could make is that having a TimerKind per Timer would establish a one to one mapping between these two types.
Yes this is also true, i think it is overall simpler to understand.
The one thing that is not supported is to have multiple actions scheduled at the same time for a single invocation because they would have the same key. Currently, we don't have this requirement. Once we have it, then we can introduce a new TimerKind for the concurrent action.
Not yet, but as described above, we already have some ideas in mind where multiple timers with same scope would be required, so I think it makes sense from now to support that.
| // without converting to i64 this field will encode as a string | ||
| // however, overflowing i64 seems unlikely | ||
| restate.internal.end_time = i64::try_from(timer_value.wake_up_time().as_u64()).expect("wake up time should fit into i64"), | ||
| ); | ||
|
|
||
| debug_if_leader!( | ||
| is_leader, | ||
| restate.timer.key = %TimerKeyDisplay(timer_value.key()), | ||
| restate.journal.index = entry_index, | ||
| restate.invocation.id = %invocation_id, |
There was a problem hiding this comment.
This is unnecessary repetition I think, you have the invocation id in the span already
There was a problem hiding this comment.
Ah ok, then I'll remove it. I was assuming it is needed because the info_span_if_leader contained it.
There was a problem hiding this comment.
A lot of other effects still have the restate.invocation.id as an event attribute. What is the guiding principle here?
There was a problem hiding this comment.
When there is a caller and callee, you put the callee invocation id too in the event itself and not the span.
There was a problem hiding this comment.
When there is a caller and callee, you put the callee invocation id too in the event itself and not the span.
I assume that for SetState there is no callee, right? It seems that we are still logging restate.invocation.id. Could it be that we didn't properly clean it up after the latest changes to the how the span is initialized?
Thanks for your review @slinkydeveloper. Could you elaborate on why you think that we need ad-hoc kinds for every type of timer we can register? From your comments, it was not fully clear to me yet. |
|
I've pushed an additional commit that introduces for every timer type its own kind. |
tillrohrmann
force-pushed
the
issues/1417
branch
from
c87da34
to
15ce0ff
Compare
|
Thanks for the review @slinkydeveloper. Merging this PR now. |
tillrohrmann
force-pushed
the
issues/1417
branch
from
15ce0ff
to
64c7193
Compare
This commit changes how we store the InvocationId in the partition storage from a custom encoding to using Protobuf. Thereby, we will be able to evolve the InvocationId in the future if needed. This fixes restatedev#1478.
This commit makes the Timer::CompleteSleepEntry contain the full InvocationId and the journal entry index to make it self-contained. This simplifies the change of the TimerKey in a follow-up commit.
The TimerKey struct now directly implements the TimerKey trait used by the timer service.
The TimerKind is used to store differently typed timers. Currently, we support journal scoped timers that are scoped to a certain journal entry. Additionally, we support invocation scoped timers that are scoped to an invocation (e.g. only a single timer for a given wake up time for a given invocation can exist). This fixes restatedev#1417.
This commit introduces factory methods on the Timer which creates the right timer kind and corresponding timer key. This prevents the accidental mixup of Timers with their keys.
tillrohrmann
force-pushed
the
issues/1417
branch
from
64c7193
to
6ea99f1
Compare
The TimerKind is used to store differently typed timers. Currently,
we support journal scoped timers that are scoped to a certain journal
entry. Additionally, we support invocation scoped timers that are scoped
to an invocation (e.g. only a single timer for a given wake up time for
a given invocation can exist).
This fixes #1417.
Only the commits starting from 6ab9fdf are relevant for this PR.