[Controller] Scheduled workflow supports catch up false #3073
Conversation
|
/lgtm |
|
BTW this is a code path that we lack backend integration tests. Not specific to this change but it would be nice to have a test coverage so we don't need manual test in the future. |
backend/api/job.proto
Outdated
| // Optional input field. Whether the job should catch up if behind schedule. | ||
| // If false, the job will only schedule the latest interval if behind schedule. | ||
| // If true, the job will catch up on each past interval. | ||
| bool catchup = 17; |
There was a problem hiding this comment.
Is there a way to specify a field in proto as default to true?
Because true is existing behavior, if this becomes default to false, then it will be a breaking change.
There was a problem hiding this comment.
I can't think of any way other than call it skip_backfill and reverse the logic?
Also do we need to expose this from UI? via something like a checkbox. If so I think you could choose the default value of such widgets.
There was a problem hiding this comment.
Yes, we can handle default logic in UI, but I kind of worry someone is calling that from python clients / http endpoints.
I'll first change the field to noCatchup then, unless we can figure out a solution.
I see, I will try adding some integration tests too. |
|
@IronPan Do you think integration tests should block this PR? |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: IronPan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
* GetNextScheduledEpochNoCatchup implementation * Add tests for cron schedule nocatchup * Add tests for periodic schedule and fix a corner case * Integrate no catchup behavior in swf controller * Update job api proto * Regenerate backend client * Pass catchup parameter in backend api * Rename proto field to no_catchup, so that it has backward compatible default value * Update generated backend api * Use no_catchup field instead * Add some comments
Backend + controller part of #3055.
Will add UI integration in separate PR.
UPDATE, I verified some basic scenarios in a real cluster.
/cc @hongye-sun
/assign @jingzhang36
/assign @IronPan
This change is