Introduce TimerKind to support different types of timers #1480

tillrohrmann · 2024-04-29T13:47:38Z

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.

slinkydeveloper

I think we need an ad-hoc kind for every type of timer we can register.

slinkydeveloper · 2024-04-29T14:24:21Z

crates/wal-protocol/src/timer.rs

+            timer_key: TimerKey::new_invocation(
+                wake_up_time.as_u64(),
+                invocation_id.invocation_uuid(),
+            ),


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.

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?

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.

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.

slinkydeveloper · 2024-04-29T14:28:03Z

crates/worker/src/partition/state_machine/effects.rs

                        // 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,


This is unnecessary repetition I think, you have the invocation id in the span already

Ah ok, then I'll remove it. I was assuming it is needed because the info_span_if_leader contained it.

A lot of other effects still have the restate.invocation.id as an event attribute. What is the guiding principle here?

When there is a caller and callee, you put the callee invocation id too in the event itself and not the span.

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?

crates/worker/src/partition/state_machine/effects.rs

tillrohrmann · 2024-04-29T14:53:34Z

I think we need an ad-hoc kind for every type of timer we can register.

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.

tillrohrmann · 2024-04-29T15:46:27Z

I've pushed an additional commit that introduces for every timer type its own kind.

slinkydeveloper

Thanks for tackling this! Can't wait to add new timers now :)

crates/storage-api/src/timer_table/mod.rs

tillrohrmann · 2024-04-30T17:45:17Z

Thanks for the review @slinkydeveloper. Merging this PR now.

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.

…nStatus

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 requested a review from slinkydeveloper April 29, 2024 13:47

slinkydeveloper reviewed Apr 29, 2024

View reviewed changes

slinkydeveloper approved these changes Apr 30, 2024

View reviewed changes

crates/storage-api/src/timer_table/mod.rs Show resolved Hide resolved

tillrohrmann force-pushed the issues/1417 branch from c87da34 to 15ce0ff Compare April 30, 2024 15:48

slinkydeveloper approved these changes Apr 30, 2024

View reviewed changes

tillrohrmann force-pushed the issues/1417 branch from 15ce0ff to 64c7193 Compare April 30, 2024 17:45

tillrohrmann added 7 commits April 30, 2024 20:45

Replace custom InvocationId encoding with Protobuf in storage

11d8c2f

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.

Make Timer::CompleteSleepEntry value self-contained

0f325fd

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.

Remove TimerKeyWrapper since it is no longer needed

2bcec0f

The TimerKey struct now directly implements the TimerKey trait used by the timer service.

Remove restate.invocation.id from timer events in effects logger

f881aa2

Introduce distinction TimerKind::Invoke and TimerKind::CleanInvocatio…

75653c5

…nStatus

Make the relationship between Timer and TimerKey more explicit

6ea99f1

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 April 30, 2024 18:45

tillrohrmann merged commit 6ea99f1 into restatedev:main Apr 30, 2024
5 checks passed

tillrohrmann deleted the issues/1417 branch April 30, 2024 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce TimerKind to support different types of timers #1480

Introduce TimerKind to support different types of timers #1480

tillrohrmann commented Apr 29, 2024 •

edited

Loading

slinkydeveloper left a comment

slinkydeveloper Apr 29, 2024

tillrohrmann Apr 29, 2024 •

edited

Loading

tillrohrmann Apr 29, 2024

slinkydeveloper Apr 30, 2024

slinkydeveloper Apr 29, 2024

tillrohrmann Apr 29, 2024 •

edited

Loading

tillrohrmann Apr 29, 2024

slinkydeveloper Apr 30, 2024

tillrohrmann Apr 30, 2024

tillrohrmann commented Apr 29, 2024

tillrohrmann commented Apr 29, 2024

slinkydeveloper left a comment

tillrohrmann commented Apr 30, 2024

Introduce TimerKind to support different types of timers #1480

Introduce TimerKind to support different types of timers #1480

Conversation

tillrohrmann commented Apr 29, 2024 • edited Loading

slinkydeveloper left a comment

Choose a reason for hiding this comment

slinkydeveloper Apr 29, 2024

Choose a reason for hiding this comment

tillrohrmann Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

tillrohrmann Apr 29, 2024

Choose a reason for hiding this comment

slinkydeveloper Apr 30, 2024

Choose a reason for hiding this comment

slinkydeveloper Apr 29, 2024

Choose a reason for hiding this comment

tillrohrmann Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

tillrohrmann Apr 29, 2024

Choose a reason for hiding this comment

slinkydeveloper Apr 30, 2024

Choose a reason for hiding this comment

tillrohrmann Apr 30, 2024

Choose a reason for hiding this comment

tillrohrmann commented Apr 29, 2024

tillrohrmann commented Apr 29, 2024

slinkydeveloper left a comment

Choose a reason for hiding this comment

tillrohrmann commented Apr 30, 2024

tillrohrmann commented Apr 29, 2024 •

edited

Loading

tillrohrmann Apr 29, 2024 •

edited

Loading

tillrohrmann Apr 29, 2024 •

edited

Loading