Flag to always retain journal #3296
Conversation
There was a problem hiding this comment.
Thanks for creating this PR @slinkydeveloper. I think the overall changes make sense to me. I've left a few comments/questions/suggestions.
crates/types/src/schema/service.rs
Outdated
| #[serde( | ||
| with = "serde_with::As::<Option<restate_serde_util::DurationString>>", | ||
| skip_serializing_if = "Option::is_none", | ||
| default | ||
| )] | ||
| #[cfg_attr(feature = "schemars", schemars(with = "Option<String>"))] | ||
| pub workflow_completion_retention: Option<Duration>, | ||
|
|
||
| /// # Journal retention | ||
| /// | ||
| /// The journal retention. This applies only to workflow calls, or to idempotent requests. | ||
| #[serde( | ||
| with = "serde_with::As::<Option<restate_serde_util::DurationString>>", | ||
| skip_serializing_if = "Option::is_none", | ||
| default | ||
| )] | ||
| #[cfg_attr(feature = "schemars", schemars(with = "Option<String>"))] | ||
| pub journal_retention: Option<Duration>, |
There was a problem hiding this comment.
Why are those fields handled differently compared to idempotency_retention? It seems that those fields won't be serialized if they are none whereas idempotency_retention will.
There was a problem hiding this comment.
Because of backward compatibility, i can't change idempotency_retention in ServiceMetadata, while these other new fields are all the same.
I added default and DefaultOnNull in ServiceMetadata.idempotency_retention, so we can in future make it optional.
There was a problem hiding this comment.
It seems that we are adding the idempotency_retention field to the HandlerMetadata in this commit. Is this field related to the ServiceMetadata somehow?
There was a problem hiding this comment.
HandlerMetadata.idempotency_retention overrides ServiceMetadata.idempotency_retention, but in theory both should have the same shape, and the default should happen at the end, and not when we store (which is what we're doing now).
…his is by default disabled, and enabled when JournalRetentionPolicy::Retain
Add business logic to remove journal when purging invocation.
This also includes few sensible changes to the internal API InvocationTargetMetadata, to map its values more easily to what the ServiceInvocation supports.
…ty timeout, abort timeout, journal retention and idempotency retention, private service
…, idempotency retention, workflow retention and private service
… the new MapAsVec with serde Add new fields in ServiceMetadata/HandlerMetadata. Introduce new ServiceMetadataResolver.resolve_invocation_attempt_timeouts
…voker. With this change, we now make sure we use for the correct deployment id, the correct inactivity/abort timeouts.
…nt_manifest + tests
To use, start with env variable `RESTATE_ADMIN__experimental_feature_force_journal_retention=1day`. Moving forward, this should be "on" by default on "dev mode".
slinkydeveloper
force-pushed
the
flag-to-always-retain-journal
branch
from
fcde01f to
984261c
Compare
There was a problem hiding this comment.
Thanks for updating this PR @slinkydeveloper. The changes look good to me. I left a comment regarding how to evolve the InvocationTargetMetadata that might make sense to resolve before merging.
| // Little problem here that can lead to some surprising stuff. | ||
| // | ||
| // When updating a service, and setting the service/handler options (e.g. the idempotency retention, private, etc) | ||
| // in the manifest, I override whatever was stored before. Makes sense. | ||
| // | ||
| // But when not setting these values, we keep whatever was configured before (either from manifest, or from Admin REST API). | ||
| // This makes little sense now that we have these settings in the manifest, but it preserves the old behavior. | ||
| // At some point we should "break" this behavior, and on service updates simply apply defaults when nothing is configured in the manifest, | ||
| // in order to favour people using annotations, rather than the clunky Admin REST API. |
There was a problem hiding this comment.
Thanks a lot for capturing this here. This will be super helpful for future me :-)
| /// In case the request has an idempotency key, the `idempotency_retention` caps the maximum `journal_retention` time. | ||
| /// In case the request targets a workflow handler, the `workflow_completion_retention` caps the maximum `journal_retention` time. |
There was a problem hiding this comment.
Oh boy. This ain't easy to understand.
| default | ||
| )] | ||
| #[cfg_attr(feature = "schemars", schemars(with = "Option<String>"))] | ||
| pub idempotency_retention: Option<Duration>, |
There was a problem hiding this comment.
Is there a difference between None and Some(Duration::Zero)?
There was a problem hiding this comment.
I assume it is for deciding whether a value was explicitly set or not when merging the configurations from the manifest with some defaults.
There was a problem hiding this comment.
yes that's the case, it's different from ServiceMetadata.idempotency_retention because in that case we apply the global default (something i would like to not do anymore....)
| pub idempotency_retention: Option<Duration>, | ||
| pub workflow_completion_retention: Option<Duration>, |
There was a problem hiding this comment.
Still wondering whether this shouldn't be simply retention and then we have some system defaults for workflow and idempotency calls which are used if no other retention duration was configured. Maybe something for the future I guess.
There was a problem hiding this comment.
at this level we could do the merge already, that's true. But I want to keep a flat data model here similar to the deployment manifest
… HandlerMetadata.
There was a problem hiding this comment.
Thanks a lot for updating the PR @slinkydeveloper. The changes look good to me. Let's get it in and benefit from it :-)
Followup of #3295
To use, start with env variable
RESTATE_ADMIN__experimental_feature_force_journal_retention=1day.Moving forward, this should be "on" by default on "dev mode".