timeskipping: add TimeSkippingConfig support to workflow-start APIs#9834
timeskipping: add TimeSkippingConfig support to workflow-start APIs#9834feiyang3cat merged 5 commits intotemporalio:mainfrom
Conversation
feiyang3cat
changed the title
feiyang3cat
force-pushed
the
ts/config-1
branch
3 times, most recently
from
1346474 to
90f833c
Compare
feiyang3cat
changed the title
feiyang3cat
changed the title
feiyang3cat
force-pushed
the
ts/config-1
branch
2 times, most recently
from
5c264c5 to
3dd6b39
Compare
feiyang3cat
changed the title
feiyang3cat
force-pushed
the
ts/config-1
branch
4 times, most recently
from
3459a5c to
8461144
Compare
feiyang3cat
changed the title
feiyang3cat
force-pushed
the
ts/config-1
branch
2 times, most recently
from
b26ae39 to
5572a38
Compare
| if timeSkippingConfig == nil { | ||
| return nil | ||
| } | ||
| if !wh.config.TimeSkippingEnabled(namespaceName.String()) { |
There was a problem hiding this comment.
What if TimeSkippingConfig is present and Enabled is set to false? Looks like this would return an error in that case. It should be ok if config.TimeSkippingEnabled is false and TimeSkippingConfig.Enabled in false.
There was a problem hiding this comment.
This is intentional — when enabled = false, the other configurations are also disallowed. Since the entire feature is flagged as unimplemented, allowing users to set these configurations would not make much sense.
There was a problem hiding this comment.
Hmm.. I wonder if we could get a default value of false in this field from any SDKs even if that was not explicitly set 🤔
And also it doesn't look right to return an error Time skipping is not enabled when the request explicitly set it to disabled.
There was a problem hiding this comment.
the error is "the time skipping feature is not enabled for this namespace"
There was a problem hiding this comment.
share my thinking for this design:
Looking at the languages supported by our SDKs, I don't think protobuf translates an optional message field to a struct initialized with default values (false/zero) in any of them. The SDKs haven't started their design yet, and I'd be surprised if they went with an approach that initializes this message field and populates it with defaults.
There's also something worth considering on the server side. If we allow configurations to be stored in the database and events for a namespace where this feature has never been enabled or implemented, it could get tricky to distinguish whether an enabled = false was deliberately set by the user or is just a default value. We'd likely need to differentiate that to make sure the default values aren't persisted and thus not visible which will be hard.
We may discuss further is that would be helpful.
There was a problem hiding this comment.
That argument makes sense. Then what if we just ignore the config if it is disabled and not return an error?
There was a problem hiding this comment.
I think that is still kind of not very simple and clean. Like we need to add code differentiate between different scenarios: (1) we also support update which cannot ignore disable (2) if the enabled is false and the bound is set, I don't know we still go a step further to patch our ignore rule or raise an error .
feiyang3cat
force-pushed
the
ts/config-1
branch
5 times, most recently
from
8e6b4ab to
349bc4c
Compare
| } | ||
| // todo: validation for max target time will need to check current virtual time for updateOptions scenario | ||
| case *workflowpb.TimeSkippingConfig_MaxTargetTime: | ||
| if bound.MaxTargetTime.AsTime().Before(time.Now().Add(namespace.MinTimeSkippingDuration)) { |
There was a problem hiding this comment.
nit: use timeSource if possible. It's in NewWorkflowHandler but not sure if the handler struct has it or now.
| // MinTimeSkippingDuration is the minimum duration for time skipping. | ||
| MinTimeSkippingDuration = 1 * time.Minute |
There was a problem hiding this comment.
nit: maybe if it configurable via dc as well in case someone needs to tune it. no strong opinion.
feiyang3cat
force-pushed
the
ts/config-1
branch
2 times, most recently
from
23e3d26 to
296a8f3
Compare
Co-authored-by: Yichao Yang <yycptt@gmail.com>
feiyang3cat
force-pushed
the
ts/config-1
branch
2 times, most recently
from
6a7d4ed to
6fdc33e
Compare
feiyang3cat
force-pushed
the
ts/config-1
branch
from
6fdc33e to
a195a35
Compare
| switch bound := timeSkippingConfig.GetBound().(type) { | ||
| case *workflowpb.TimeSkippingConfig_MaxSkippedDuration: | ||
| if bound.MaxSkippedDuration.AsDuration() < namespace.MinTimeSkippingDuration { | ||
| return serviceerror.NewUnimplementedf( |
There was a problem hiding this comment.
@feiyang3cat, sorry coming late to this. This should be NewInvalidArgumentf and the same for below
4 participants
What changed?
Add time-skipping configuration support.
(1) Feature flag in dynamic config
- frontend.TimeSkippingEnabled (namespace bool, default false) gates the feature
(2) Frontend validation (validateTimeSkippingConfig)
- StartWorkflowExecution
- SignalWithStartWorkflowExecution
- ExecuteMultiOperation (via the shared prepareStartWorkflowRequest path)
(3) Persistence
- TimeSkippingInfo proto added to WorkflowExecutionInfo
- MutableState stores the config when a workflow starts or options are updated
(4) Tests
- New timeskipping functional tests in tests/timeskipping_test.go
Why?
first step of the time skipping project
How did you test it?