Surface vqueues_enabled via StateMachineFeatures#4772
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| debug_if_leader!(ctx.is_leader, "Paused the invocation"); | ||
|
|
||
| if invoked_meta.vqueue_id.is_some() { | ||
| if ctx.is_vqueues_enabled() { |
There was a problem hiding this comment.
I think I've jumped a little bit too far ahead and assumed the migration of all existing invocations and therefore put the ctx.is_vqueues_enabled() check. Will revert it.
| .timestamps | ||
| .update(ctx.record_created_at); | ||
|
|
||
| if Configuration::pinned() |
There was a problem hiding this comment.
seems like this is based off an old base commit
| } | ||
|
|
||
| impl<S> StateMachineFeatures for StateMachineApplyContext<'_, S> { | ||
| fn use_journal_v2_as_default(&self) -> bool { |
There was a problem hiding this comment.
given that restate version is unlikely to change after the state machine starts, does it make sense to do this comparison dynamically at call time?
There was a problem hiding this comment.
I believe that we need this for the transition from v1.6 to v1.7 to coordinate when exactly we start using the journal v2 across all replicas. In v1.8, this can become a static check. The way I can improve it is to store it as a feature in the PersistedStateMachineFeatures and not doing a SemanticRestateVersion comparison.
tillrohrmann
force-pushed
the
state-machine-features
branch
3 times, most recently
from
a451d3b to
bd250c2
Compare
|
@codex review |
| Command::VersionBarrier(VersionBarrierCommand { | ||
| version: RESTATE_VERSION_1_6_0.clone(), | ||
| // for backwards compatibility with v1.6 we need to set the version to 1.6.0-dev | ||
| version: PartitionFeatureChange::EnableJournalV2 |
There was a problem hiding this comment.
Is it possible to be in a situation where a version barrier write moves the enforced version backwards?
There was a problem hiding this comment.
We are only updating the stored min restate version if the version in the barrier is greater than what the current min_restate_version is. But we can add min_restate_version.max() as a safety measure.
| fn use_journal_v2_as_default(&self) -> bool { | ||
| // use journal v2 as default if the min Restate version is >= v1.6.0 | ||
| self.is_equal_or_newer_than(&RESTATE_VERSION_1_6_0) | ||
| self.enabled_features.journal_v2 |
AhmedSoliman
left a comment
AhmedSoliman
left a comment
There was a problem hiding this comment.
I've to admit that some details are a little confusing to me. I'll trust that you have done sufficient testing on this.
What are the things that are confusing you? |
Extend the StateMachineFeatures trait with is_vqueues_enabled (gated on the persisted feature flag) and move the impl from SemanticRestateVersion to StateMachine + StateMachineApplyContext so each method can consult both the min Restate version and the persisted feature set. Replace the in-state-machine Configuration::pinned().common.experimental.is_vqueues_enabled() call sites with self.is_vqueues_enabled() / ctx.is_vqueues_enabled(). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tillrohrmann
force-pushed
the
state-machine-features
branch
from
bd250c2 to
9d01730
Compare
2 participants
Extend the StateMachineFeatures trait with is_vqueues_enabled (gated on the
persisted feature flag) and move the impl from SemanticRestateVersion to
StateMachine + StateMachineApplyContext so each method can consult both the
min Restate version and the persisted feature set. Replace the in-state-machine
Configuration::pinned().common.experimental.is_vqueues_enabled() call sites
with self.is_vqueues_enabled() / ctx.is_vqueues_enabled().
The current approach is quite simple. If we need the knowledge about enabled features at other places like the storage layer, we might want to think about a more generic access pattern.
This PR is based on #4770.