[Scheduler] Add a new flag to allow only migration of schedules without running workflows#10505
Conversation
…ut running workflows
| if s.tweakables.EnableCHASMMigration { | ||
| s.State.PendingMigration = true | ||
| } | ||
| if s.State.PendingMigration { | ||
| if s.State.PendingMigration && | ||
| (s.tweakables.MigrateWithRunningWorkflows || len(s.Info.RunningWorkflows) == 0) && | ||
| // re-check that EnableCHASMMigration is still true for the namespace's config | ||
| s.tweakables.EnableCHASMMigration { |
There was a problem hiding this comment.
if !s.State.PendingMigration && s.tweakables.EnableCHASMMigration &&
(s.tweakables.MigrateWithRunningWorkflows || len(s.Info.RunningWorkflows) {
s.State.PendingMigration = true
}
if s.State.PendingMigration {
err := s.executeMigration()
...
}
what do you thin about this?
There was a problem hiding this comment.
What about plumbing all these conditionals into the activity, so that we don't have to worry about nondeterminitism?
There was a problem hiding this comment.
Changes to all of these variables are deterministic because we read them through MutableSideEffect (tweakables), or Signal into the workflow.
this suggested change is mostly to prevent changes in behavior given the same input and simplify it (at least in my head)
There was a problem hiding this comment.
it would also still support migrations via signal.
There was a problem hiding this comment.
My thought about doing the eval in the activity is that it allows for instantaneous update, rather than capturing a side-effect in history earlier, which then must be honoured, irrespective of dynamic config's current state. Ie, it'd allow for a faster response time if DC changed value.
But re the logic, I'm not sure I understand it, to Alex's point, I'm not sure why EnableCHASMMigration is being evaluated on line 337 and 331?
There was a problem hiding this comment.
why EnableCHASMMigration is being evaluated on line 337 and 331?
it would allow the migration flag going from true to false to to halt the migration of a schedule that is already in progress.
I'm happy with the changes @lina-temporal just pushed.
re using an activity: I'd rather read through a mutable side effect because it's an existing pattern in this workflow and we've test it quite extensively already.
What changed?
Why?
How did you test it?
Potential risks