fix time skipping bugs in updateWfOptions#10669
Conversation
feiyang3cat
force-pushed
the
ts-fix-update-wf-options
branch
3 times, most recently
from
4102314 to
f0e724a
Compare
| TimeSkippingInfo: &persistencespb.TimeSkippingInfo{Config: tc.currentTsc}, | ||
| }).AnyTimes() | ||
| // TSC arg must be nil — updateTimeSkippingInfo must NOT be called. | ||
| ms.EXPECT().AddWorkflowExecutionOptionsUpdatedEvent( |
There was a problem hiding this comment.
@simvlad the test case to make sure unrelated field changes will have a nil value for the time skipping config
-> in service/history/workflow/mutable_state_impl_test.go TestAddWorkflowExecutionOptionsUpdatedEvent_NilTimeSkippingConfig
ensures nil TSC has no side effects
| @@ -123,6 +123,16 @@ func Invoke( | |||
| return ret, nil | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
adding this notion of "option with side effect" to highlight that not all options can be simply merged and applied, and I think it is better to just call is time skipping config and mark it as an exception
feiyang3cat
force-pushed
the
ts-fix-update-wf-options
branch
from
f0e724a to
3fabf30
Compare
| sideEffects.timeSkippingConfig = true | ||
| } else { | ||
| sideEffects.timeSkippingConfig = false | ||
| mergeInto.TimeSkippingConfig = nil |
There was a problem hiding this comment.
Wouldn't this clear the TimeSkippingConfig, when an update doesn't set it (i.e. updates just a version)
There was a problem hiding this comment.
Not sure if I follow:
It won't as I commented in the mutable_state_impl.go nil TSC in the updateWorkflowOptions won't trigger a call to update workflow options
it is tested in TestAddWorkflowExecutionOptionsUpdatedEvent_NilTimeSkippingConfig
There was a problem hiding this comment.
oh, I think using "the mutable_state_impl.go nil TSC in the updateWorkflowOptions won't trigger a call to update workflow options" this strategy is not good as we allow users to change it to nil
I changed the design here pls take a look at the latest commit.
but I agree the new one looks tricky as it is hard to fit the side-effect into the "merge and compare" implementation
| if _, ok := updateFields["timeSkippingConfig"]; ok { | ||
| if mergeFrom.GetTimeSkippingConfig() != nil { | ||
| mergeInto.TimeSkippingConfig = mergeFrom.GetTimeSkippingConfig() | ||
| // - has side effects when applied so equality check is not enough to determine if there are changes; |
There was a problem hiding this comment.
calling attention for this one @simvlad
and the corresponding api change temporalio/api@c5d728b
feiyang3cat
force-pushed
the
ts-fix-update-wf-options
branch
4 times, most recently
from
778eeef to
112bf89
Compare
and refine tests for update workflow options
feiyang3cat
force-pushed
the
ts-fix-update-wf-options
branch
from
4e040ea to
5ae83f4
Compare
2 participants
What changed and why
UpdateWorkflowExecutionOptions uses a merge logic where the requested fields overwrite the current
options. This does not fit TimeSkippingConfig, which has side effects when applied (a timer task is
emitted and the bound target time is recalculated). We need a way to distinguish whether the config is changed.
We therefore add side-effect update logic with two rules:
▎ 1. The workflow options event carries a nil TSC when time skipping is not being updated.
▎ 2. Whenever the bound is explicitly set in the request, it is always treated as a change — even if the
▎ value is the same as the current one — to renew the timer task and target time.
How did you test it?