From 4e41e51f22d6853771e5f13a6f0cc0795a26fae7 Mon Sep 17 00:00:00 2001 From: Yuan Gong Date: Thu, 20 Feb 2020 12:59:05 +0800 Subject: [PATCH 1/6] Update codegen instruction --- frontend/README.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/frontend/README.md b/frontend/README.md index 8bdaeb6e8a4..41d080f23e3 100644 --- a/frontend/README.md +++ b/frontend/README.md @@ -2,12 +2,13 @@ ## Develop -You need `npm`, install dependencies using `npm install`. +You need `npm`, install dependencies using `npm ci` (`npm ci` makes sure your +installed dependencies have the exact same version as others). If you made any changes to protos (see backend/README), you'll need to regenerate the Typescript client library from swagger. We use swagger-codegen-cli@2.4.7, which you can get -[here](http://central.maven.org/maven2/io/swagger/swagger-codegen-cli/2.4.7/swagger-codegen-cli-2.4.7.jar). +[here](https://repo1.maven.org/maven2/io/swagger/swagger-codegen-cli/2.4.7/). Make sure the jar file is somewhere on your path with the name swagger-codegen-cli.jar, then run `npm run apis`. From 10892445ce5534fd266fdff4a281ab4d181bfb61 Mon Sep 17 00:00:00 2001 From: Yuan Gong Date: Thu, 20 Feb 2020 12:59:14 +0800 Subject: [PATCH 2/6] Regenerate api --- frontend/src/apis/job/api.ts | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/frontend/src/apis/job/api.ts b/frontend/src/apis/job/api.ts index 8f0ffb5cc8a..7036625cdd2 100644 --- a/frontend/src/apis/job/api.ts +++ b/frontend/src/apis/job/api.ts @@ -191,6 +191,12 @@ export interface ApiJob { * @memberof ApiJob */ enabled?: boolean; + /** + * Optional input field. Whether the job should catch up if behind schedule. If true, the job will only schedule the latest interval if behind schedule. If false, the job will catch up on each past interval. + * @type {boolean} + * @memberof ApiJob + */ + no_catchup?: boolean; } /** @@ -558,7 +564,7 @@ export const JobServiceApiFetchParamCreator = function(configuration?: Configura }, /** * - * @summary Disable a job. + * @summary Stops a job and all its associated runs. The job is not deleted. * @param {string} id The ID of the job to be disabled * @param {*} [options] Override http request option. * @throws {RequiredError} @@ -606,7 +612,7 @@ export const JobServiceApiFetchParamCreator = function(configuration?: Configura }, /** * - * @summary Enable a job. + * @summary Restarts a job that was previously stopped. All runs associated with the job will continue. * @param {string} id The ID of the job to be enabled * @param {*} [options] Override http request option. * @throws {RequiredError} @@ -839,7 +845,7 @@ export const JobServiceApiFp = function(configuration?: Configuration) { }, /** * - * @summary Disable a job. + * @summary Stops a job and all its associated runs. The job is not deleted. * @param {string} id The ID of the job to be disabled * @param {*} [options] Override http request option. * @throws {RequiredError} @@ -861,7 +867,7 @@ export const JobServiceApiFp = function(configuration?: Configuration) { }, /** * - * @summary Enable a job. + * @summary Restarts a job that was previously stopped. All runs associated with the job will continue. * @param {string} id The ID of the job to be enabled * @param {*} [options] Override http request option. * @throws {RequiredError} @@ -981,7 +987,7 @@ export const JobServiceApiFactory = function( }, /** * - * @summary Disable a job. + * @summary Stops a job and all its associated runs. The job is not deleted. * @param {string} id The ID of the job to be disabled * @param {*} [options] Override http request option. * @throws {RequiredError} @@ -991,7 +997,7 @@ export const JobServiceApiFactory = function( }, /** * - * @summary Enable a job. + * @summary Restarts a job that was previously stopped. All runs associated with the job will continue. * @param {string} id The ID of the job to be enabled * @param {*} [options] Override http request option. * @throws {RequiredError} @@ -1082,7 +1088,7 @@ export class JobServiceApi extends BaseAPI { /** * - * @summary Disable a job. + * @summary Stops a job and all its associated runs. The job is not deleted. * @param {string} id The ID of the job to be disabled * @param {*} [options] Override http request option. * @throws {RequiredError} @@ -1094,7 +1100,7 @@ export class JobServiceApi extends BaseAPI { /** * - * @summary Enable a job. + * @summary Restarts a job that was previously stopped. All runs associated with the job will continue. * @param {string} id The ID of the job to be enabled * @param {*} [options] Override http request option. * @throws {RequiredError} From ee75822c7747b3896bc1a3d34a217390b2bbca92 Mon Sep 17 00:00:00 2001 From: Yuan Gong Date: Thu, 20 Feb 2020 13:57:59 +0800 Subject: [PATCH 3/6] [UI] scheduled workflow catchup option --- frontend/src/components/Trigger.tsx | 44 +++++++++++++++----- frontend/src/pages/NewRun.tsx | 48 +++++++++++----------- frontend/src/pages/RecurringRunDetails.tsx | 3 ++ 3 files changed, 60 insertions(+), 35 deletions(-) diff --git a/frontend/src/components/Trigger.tsx b/frontend/src/components/Trigger.tsx index 485e502a5d9..b3f80305d21 100644 --- a/frontend/src/components/Trigger.tsx +++ b/frontend/src/components/Trigger.tsx @@ -35,7 +35,11 @@ import { ApiTrigger } from '../apis/job'; import { stylesheet } from 'typestyle'; interface TriggerProps { - onChange?: (trigger?: ApiTrigger, maxConcurrentRuns?: string) => void; + onChange?: (config: { + trigger?: ApiTrigger; + maxConcurrentRuns?: string; + catchup: boolean; + }) => void; } interface TriggerState { @@ -52,6 +56,7 @@ interface TriggerState { startDate: string; startTime: string; type: TriggerType; + catchup: boolean; } const css = stylesheet({ @@ -61,9 +66,7 @@ const css = stylesheet({ }); export default class Trigger extends React.Component { - constructor(props: any) { - super(props); - + public state = (() => { const now = new Date(); const inAWeek = new Date( now.getFullYear(), @@ -75,7 +78,7 @@ export default class Trigger extends React.Component const [startDate, startTime] = dateToPickerFormat(now); const [endDate, endTime] = dateToPickerFormat(inAWeek); - this.state = { + return { cron: '', editCron: false, endDate, @@ -89,8 +92,9 @@ export default class Trigger extends React.Component startDate, startTime, type: TriggerType.INTERVALED, + catchup: true, }; - } + })(); public componentDidMount(): void { // TODO: This is called here because NewRun only updates its Trigger in state when onChange is @@ -114,6 +118,7 @@ export default class Trigger extends React.Component startDate, startTime, type, + catchup, } = this.state; return ( @@ -209,6 +214,18 @@ export default class Trigger extends React.Component variant='outlined' /> + + + } + label='Catchup' + /> + Run every @@ -322,11 +339,11 @@ export default class Trigger extends React.Component { [name]: value, } as any, - this._updateTrigger.bind(this), + this._updateTrigger, ); }; - private _updateTrigger(): void { + private _updateTrigger = () => { const { hasStartDate, hasEndDate, @@ -340,6 +357,7 @@ export default class Trigger extends React.Component type, cron, selectedDays, + catchup, } = this.state; const startDateTime = pickersToDate(hasStartDate, startDate, startTime); @@ -363,11 +381,15 @@ export default class Trigger extends React.Component ); if (this.props.onChange) { - this.props.onChange(trigger, trigger ? this.state.maxConcurrentRuns : undefined); + this.props.onChange({ + trigger, + maxConcurrentRuns: trigger ? this.state.maxConcurrentRuns : undefined, + catchup, + }); } }, ); - } + }; private _isAllDaysChecked(): boolean { return this.state.selectedDays.every(d => !!d); @@ -397,7 +419,7 @@ export default class Trigger extends React.Component cron, selectedDays: newDays, }, - this._updateTrigger.bind(this), + this._updateTrigger, ); } } diff --git a/frontend/src/pages/NewRun.tsx b/frontend/src/pages/NewRun.tsx index 86482a20838..22c55883d64 100644 --- a/frontend/src/pages/NewRun.tsx +++ b/frontend/src/pages/NewRun.tsx @@ -68,6 +68,7 @@ interface NewRunState { isFirstRunInExperiment: boolean; isRecurringRun: boolean; maxConcurrentRuns?: string; + catchup: boolean; parameters: ApiParameter[]; pipeline?: ApiPipeline; pipelineVersion?: ApiPipelineVersion; @@ -142,29 +143,26 @@ class NewRun extends Page<{}, NewRunState> { { label: 'Created at', flex: 1, sortKey: ExperimentSortKeys.CREATED_AT }, ]; - constructor(props: any) { - super(props); - - this.state = { - description: '', - errorMessage: '', - experimentName: '', - experimentSelectorOpen: false, - isBeingStarted: false, - isClone: false, - isFirstRunInExperiment: false, - isRecurringRun: false, - parameters: [], - pipelineName: '', - pipelineSelectorOpen: false, - pipelineVersionName: '', - pipelineVersionSelectorOpen: false, - runName: '', - uploadDialogOpen: false, - usePipelineFromRunLabel: 'Using pipeline from cloned run', - useWorkflowFromRun: false, - }; - } + public state: NewRunState = { + description: '', + errorMessage: '', + experimentName: '', + experimentSelectorOpen: false, + isBeingStarted: false, + isClone: false, + isFirstRunInExperiment: false, + isRecurringRun: false, + parameters: [], + pipelineName: '', + pipelineSelectorOpen: false, + pipelineVersionName: '', + pipelineVersionSelectorOpen: false, + runName: '', + uploadDialogOpen: false, + usePipelineFromRunLabel: 'Using pipeline from cloned run', + useWorkflowFromRun: false, + catchup: true, // defaults to true + }; public getInitialToolbarState(): ToolbarProps { return { @@ -507,11 +505,12 @@ class NewRun extends Page<{}, NewRunState> {
Choose a method by which new runs will be triggered
+ onChange={({ trigger, maxConcurrentRuns, catchup }) => this.setStateSafe( { maxConcurrentRuns, trigger, + catchup, }, this._validate.bind(this), ) @@ -1037,6 +1036,7 @@ class NewRun extends Page<{}, NewRunState> { enabled: true, max_concurrency: this.state.maxConcurrentRuns || '1', trigger: this.state.trigger, + no_catchup: !this.state.catchup, }); } diff --git a/frontend/src/pages/RecurringRunDetails.tsx b/frontend/src/pages/RecurringRunDetails.tsx index cfe1e95b21a..37a417d5575 100644 --- a/frontend/src/pages/RecurringRunDetails.tsx +++ b/frontend/src/pages/RecurringRunDetails.tsx @@ -104,6 +104,9 @@ class RecurringRunDetails extends Page<{}, RecurringRunConfigState> { formatDateString(run.trigger.periodic_schedule.end_time), ]); } + if (run.no_catchup != null) { + triggerDetails.push(['Catchup', `${run.no_catchup}`]); + } } } From 33661ca7378e77274033858f1b5746a7f0dbd7d4 Mon Sep 17 00:00:00 2001 From: Yuan Gong Date: Thu, 20 Feb 2020 18:46:29 +0800 Subject: [PATCH 4/6] Show catchup in recurring run details page --- frontend/src/pages/RecurringRunDetails.tsx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/frontend/src/pages/RecurringRunDetails.tsx b/frontend/src/pages/RecurringRunDetails.tsx index 37a417d5575..3144ab07f92 100644 --- a/frontend/src/pages/RecurringRunDetails.tsx +++ b/frontend/src/pages/RecurringRunDetails.tsx @@ -85,6 +85,7 @@ class RecurringRunDetails extends Page<{}, RecurringRunConfigState> { if (run.max_concurrency) { triggerDetails.push(['Max. concurrent runs', run.max_concurrency]); } + triggerDetails.push(['Catchup', `${run.no_catchup || false}`]); if (run.trigger.cron_schedule && run.trigger.cron_schedule.start_time) { triggerDetails.push([ 'Start time', @@ -104,9 +105,6 @@ class RecurringRunDetails extends Page<{}, RecurringRunConfigState> { formatDateString(run.trigger.periodic_schedule.end_time), ]); } - if (run.no_catchup != null) { - triggerDetails.push(['Catchup', `${run.no_catchup}`]); - } } } From 0362bada1c008e4447e53f609655c841b866e533 Mon Sep 17 00:00:00 2001 From: Yuan Gong Date: Thu, 20 Feb 2020 19:52:51 +0800 Subject: [PATCH 5/6] Add help button to introduce swf catchup=false --- frontend/src/atoms/HelpButton.tsx | 40 +++++++++++++++++++++++++++++ frontend/src/components/Trigger.tsx | 37 +++++++++++++++++++------- frontend/src/pages/NewRun.tsx | 2 +- 3 files changed, 69 insertions(+), 10 deletions(-) create mode 100644 frontend/src/atoms/HelpButton.tsx diff --git a/frontend/src/atoms/HelpButton.tsx b/frontend/src/atoms/HelpButton.tsx new file mode 100644 index 00000000000..e82bcbecf26 --- /dev/null +++ b/frontend/src/atoms/HelpButton.tsx @@ -0,0 +1,40 @@ +import Card from '@material-ui/core/Card'; +import CardContent from '@material-ui/core/CardContent'; +import IconButton from '@material-ui/core/IconButton'; +import { withStyles } from '@material-ui/core/styles'; +import Tooltip from '@material-ui/core/Tooltip'; +import HelpIcon from '@material-ui/icons/Help'; +import React, { ReactNode } from 'react'; +import { color, fontsize } from 'src/Css'; + +const NostyleTooltip = withStyles({ + tooltip: { + backgroundColor: 'transparent', + maxWidth: 220, + fontSize: fontsize.base, + color: color.secondaryText, + border: '0 none', + }, +})(Tooltip); + +interface HelpButtonProps { + helpText?: ReactNode; +} +export const HelpButton: React.FC = ({ helpText }) => { + return ( + + {helpText} + + } + interactive + leaveDelay={400} + placement='top' + > + + + + + ); +}; diff --git a/frontend/src/components/Trigger.tsx b/frontend/src/components/Trigger.tsx index b3f80305d21..fd5bb83bb81 100644 --- a/frontend/src/components/Trigger.tsx +++ b/frontend/src/components/Trigger.tsx @@ -14,25 +14,26 @@ * limitations under the License. */ -import * as React from 'react'; import Button from '@material-ui/core/Button'; import Checkbox from '@material-ui/core/Checkbox'; import FormControlLabel from '@material-ui/core/FormControlLabel'; -import Input from '../atoms/Input'; import MenuItem from '@material-ui/core/MenuItem'; +import * as React from 'react'; +import { stylesheet } from 'typestyle'; +import { ApiTrigger } from '../apis/job'; +import { HelpButton } from '../atoms/HelpButton'; +import Input from '../atoms/Input'; import Separator from '../atoms/Separator'; import { commonCss } from '../Css'; -import { dateToPickerFormat } from '../lib/TriggerUtils'; import { - PeriodicInterval, - TriggerType, - triggers, buildCron, - pickersToDate, buildTrigger, + dateToPickerFormat, + PeriodicInterval, + pickersToDate, + triggers, + TriggerType, } from '../lib/TriggerUtils'; -import { ApiTrigger } from '../apis/job'; -import { stylesheet } from 'typestyle'; interface TriggerProps { onChange?: (config: { @@ -225,6 +226,24 @@ export default class Trigger extends React.Component } label='Catchup' /> + +

+ Whether the recurring run should catch up if behind schedule. Defaults to true. +

+

+ For example, if the recurring run is paused for a while and re-enabled + afterwards. If catchup=true, the scheduler will catch up on (backfill) each + missed interval. Otherwise, it only schedules the latest interval. +

+

+ Usually, if your pipeline handles backfill internally, you should turn catchup + off to avoid duplicate work. +

+ + } + /> diff --git a/frontend/src/pages/NewRun.tsx b/frontend/src/pages/NewRun.tsx index 22c55883d64..4e2c16c7618 100644 --- a/frontend/src/pages/NewRun.tsx +++ b/frontend/src/pages/NewRun.tsx @@ -161,7 +161,7 @@ class NewRun extends Page<{}, NewRunState> { uploadDialogOpen: false, usePipelineFromRunLabel: 'Using pipeline from cloned run', useWorkflowFromRun: false, - catchup: true, // defaults to true + catchup: true, }; public getInitialToolbarState(): ToolbarProps { From 46ae4fcfa92d847403a6973e3f44045fa111073f Mon Sep 17 00:00:00 2001 From: Yuan Gong Date: Fri, 21 Feb 2020 10:35:42 +0800 Subject: [PATCH 6/6] Update snapshots and fix tests --- frontend/src/atoms/HelpButton.tsx | 10 +- frontend/src/components/Trigger.test.tsx | 199 ++++++++++-------- frontend/src/components/Trigger.tsx | 6 +- .../__snapshots__/Trigger.test.tsx.snap | 145 +++++++++++++ frontend/src/lib/Apis.ts | 2 +- frontend/src/lib/RunUtils.ts | 2 +- frontend/src/pages/GettingStarted.test.tsx | 2 +- frontend/src/pages/NewRun.test.tsx | 1 + frontend/src/pages/NewRun.tsx | 46 ++-- .../src/pages/RecurringRunDetails.test.tsx | 27 ++- frontend/src/pages/RecurringRunDetails.tsx | 2 +- .../RecurringRunDetails.test.tsx.snap | 8 + 12 files changed, 312 insertions(+), 138 deletions(-) diff --git a/frontend/src/atoms/HelpButton.tsx b/frontend/src/atoms/HelpButton.tsx index e82bcbecf26..e834a7c36d5 100644 --- a/frontend/src/atoms/HelpButton.tsx +++ b/frontend/src/atoms/HelpButton.tsx @@ -5,15 +5,15 @@ import { withStyles } from '@material-ui/core/styles'; import Tooltip from '@material-ui/core/Tooltip'; import HelpIcon from '@material-ui/icons/Help'; import React, { ReactNode } from 'react'; -import { color, fontsize } from 'src/Css'; +import { color, fontsize } from '../Css'; const NostyleTooltip = withStyles({ tooltip: { backgroundColor: 'transparent', - maxWidth: 220, - fontSize: fontsize.base, - color: color.secondaryText, border: '0 none', + color: color.secondaryText, + fontSize: fontsize.base, + maxWidth: 220, }, })(Tooltip); @@ -28,7 +28,7 @@ export const HelpButton: React.FC = ({ helpText }) => { {helpText} } - interactive + interactive={true} leaveDelay={400} placement='top' > diff --git a/frontend/src/components/Trigger.test.tsx b/frontend/src/components/Trigger.test.tsx index 3d10c5b3382..b72ab6e2a6b 100644 --- a/frontend/src/components/Trigger.test.tsx +++ b/frontend/src/components/Trigger.test.tsx @@ -19,6 +19,17 @@ import Trigger from './Trigger'; import { shallow } from 'enzyme'; import { TriggerType, PeriodicInterval } from '../lib/TriggerUtils'; +const PARAMS_DEFAULT = { + catchup: true, + maxConcurrentRuns: '10', +}; +const PERIODIC_DEFAULT = { + end_time: undefined, + interval_second: '60', + start_time: undefined, +}; +const CRON_DEFAULT = { cron: '0 * * * * ?', end_time: undefined, start_time: undefined }; + describe('Trigger', () => { // tslint:disable-next-line:variable-name const RealDate = Date; @@ -82,12 +93,12 @@ describe('Trigger', () => { (tree.instance() as Trigger).handleChange('type')({ target: { value: TriggerType.INTERVALED }, }); - expect(spy).toHaveBeenLastCalledWith( - { - periodic_schedule: { end_time: undefined, interval_second: '60', start_time: undefined }, + expect(spy).toHaveBeenLastCalledWith({ + ...PARAMS_DEFAULT, + trigger: { + periodic_schedule: PERIODIC_DEFAULT, }, - '10', - ); + }); }); it('builds trigger with a start time if the checkbox is checked', () => { @@ -99,10 +110,12 @@ describe('Trigger', () => { (tree.instance() as Trigger).handleChange('hasStartDate')({ target: { type: 'checkbox', checked: true }, }); - expect(spy).toHaveBeenLastCalledWith( - { periodic_schedule: { end_time: undefined, interval_second: '60', start_time: testDate } }, - '10', - ); + expect(spy).toHaveBeenLastCalledWith({ + ...PARAMS_DEFAULT, + trigger: { + periodic_schedule: { ...PERIODIC_DEFAULT, start_time: testDate }, + }, + }); }); it('builds trigger with the entered start date/time', () => { @@ -116,16 +129,15 @@ describe('Trigger', () => { }); (tree.instance() as Trigger).handleChange('startDate')({ target: { value: '2018-11-23' } }); (tree.instance() as Trigger).handleChange('endTime')({ target: { value: '08:35' } }); - expect(spy).toHaveBeenLastCalledWith( - { + expect(spy).toHaveBeenLastCalledWith({ + ...PARAMS_DEFAULT, + trigger: { periodic_schedule: { - end_time: undefined, - interval_second: '60', + ...PERIODIC_DEFAULT, start_time: new Date(2018, 10, 23, 8, 35), }, }, - '10', - ); + }); }); it('builds trigger without the entered start date if no time is entered', () => { @@ -139,16 +151,12 @@ describe('Trigger', () => { }); (tree.instance() as Trigger).handleChange('startDate')({ target: { value: '2018-11-23' } }); (tree.instance() as Trigger).handleChange('startTime')({ target: { value: '' } }); - expect(spy).toHaveBeenLastCalledWith( - { - periodic_schedule: { - end_time: undefined, - interval_second: '60', - start_time: undefined, - }, + expect(spy).toHaveBeenLastCalledWith({ + ...PARAMS_DEFAULT, + trigger: { + periodic_schedule: PERIODIC_DEFAULT, }, - '10', - ); + }); }); it('builds trigger without the entered start time if no date is entered', () => { @@ -162,16 +170,12 @@ describe('Trigger', () => { }); (tree.instance() as Trigger).handleChange('startDate')({ target: { value: '' } }); (tree.instance() as Trigger).handleChange('startTime')({ target: { value: '11:33' } }); - expect(spy).toHaveBeenLastCalledWith( - { - periodic_schedule: { - end_time: undefined, - interval_second: '60', - start_time: undefined, - }, + expect(spy).toHaveBeenLastCalledWith({ + ...PARAMS_DEFAULT, + trigger: { + periodic_schedule: PERIODIC_DEFAULT, }, - '10', - ); + }); }); it('builds trigger with a date if both start and end checkboxes are checked', () => { @@ -186,10 +190,12 @@ describe('Trigger', () => { (tree.instance() as Trigger).handleChange('hasEndDate')({ target: { type: 'checkbox', checked: true }, }); - expect(spy).toHaveBeenLastCalledWith( - { periodic_schedule: { end_time: testDate, interval_second: '60', start_time: testDate } }, - '10', - ); + expect(spy).toHaveBeenLastCalledWith({ + ...PARAMS_DEFAULT, + trigger: { + periodic_schedule: { ...PERIODIC_DEFAULT, end_time: testDate, start_time: testDate }, + }, + }); }); it('resets trigger to no start date if it is added then removed', () => { @@ -204,12 +210,12 @@ describe('Trigger', () => { (tree.instance() as Trigger).handleChange('hasStartDate')({ target: { type: 'checkbox', checked: false }, }); - expect(spy).toHaveBeenLastCalledWith( - { - periodic_schedule: { end_time: undefined, interval_second: '60', start_time: undefined }, + expect(spy).toHaveBeenLastCalledWith({ + ...PARAMS_DEFAULT, + trigger: { + periodic_schedule: PERIODIC_DEFAULT, }, - '10', - ); + }); }); it('builds trigger with a weekly interval', () => { @@ -221,16 +227,15 @@ describe('Trigger', () => { (tree.instance() as Trigger).handleChange('intervalCategory')({ target: { value: PeriodicInterval.WEEK }, }); - expect(spy).toHaveBeenLastCalledWith( - { + expect(spy).toHaveBeenLastCalledWith({ + ...PARAMS_DEFAULT, + trigger: { periodic_schedule: { - end_time: undefined, + ...PERIODIC_DEFAULT, interval_second: (7 * 24 * 60 * 60).toString(), - start_time: undefined, }, }, - '10', - ); + }); }); it('builds trigger with an every-three-months interval', () => { @@ -243,16 +248,15 @@ describe('Trigger', () => { target: { value: PeriodicInterval.MONTH }, }); (tree.instance() as Trigger).handleChange('intervalValue')({ target: { value: 3 } }); - expect(spy).toHaveBeenLastCalledWith( - { + expect(spy).toHaveBeenLastCalledWith({ + ...PARAMS_DEFAULT, + trigger: { periodic_schedule: { - end_time: undefined, + ...PERIODIC_DEFAULT, interval_second: (3 * 30 * 24 * 60 * 60).toString(), - start_time: undefined, }, }, - '10', - ); + }); }); it('builds trigger with the specified max concurrency setting', () => { @@ -262,16 +266,31 @@ describe('Trigger', () => { target: { value: TriggerType.INTERVALED }, }); (tree.instance() as Trigger).handleChange('maxConcurrentRuns')({ target: { value: '3' } }); - expect(spy).toHaveBeenLastCalledWith( - { - periodic_schedule: { - end_time: undefined, - interval_second: '60', - start_time: undefined, - }, + expect(spy).toHaveBeenLastCalledWith({ + ...PARAMS_DEFAULT, + maxConcurrentRuns: '3', + trigger: { + periodic_schedule: PERIODIC_DEFAULT, + }, + }); + }); + + it('builds trigger with the specified catchup setting', () => { + const spy = jest.fn(); + const tree = shallow(); + (tree.instance() as Trigger).handleChange('type')({ + target: { value: TriggerType.INTERVALED }, + }); + (tree.instance() as Trigger).handleChange('catchup')({ + target: { type: 'checkbox', checked: false }, + }); + expect(spy).toHaveBeenLastCalledWith({ + ...PARAMS_DEFAULT, + catchup: false, + trigger: { + periodic_schedule: PERIODIC_DEFAULT, }, - '3', - ); + }); }); }); @@ -280,10 +299,12 @@ describe('Trigger', () => { const spy = jest.fn(); const tree = shallow(); (tree.instance() as Trigger).handleChange('type')({ target: { value: TriggerType.CRON } }); - expect(spy).toHaveBeenLastCalledWith( - { cron_schedule: { cron: '0 * * * * ?', end_time: undefined, start_time: undefined } }, - '10', - ); + expect(spy).toHaveBeenLastCalledWith({ + ...PARAMS_DEFAULT, + trigger: { + cron_schedule: CRON_DEFAULT, + }, + }); }); it('builds a 1-minute cron trigger with specified start date', () => { @@ -294,10 +315,12 @@ describe('Trigger', () => { target: { type: 'checkbox', checked: true }, }); (tree.instance() as Trigger).handleChange('startDate')({ target: { value: '2018-03-23' } }); - expect(spy).toHaveBeenLastCalledWith( - { cron_schedule: { cron: '0 * * * * ?', end_time: undefined, start_time: testDate } }, - '10', - ); + expect(spy).toHaveBeenLastCalledWith({ + ...PARAMS_DEFAULT, + trigger: { + cron_schedule: { ...CRON_DEFAULT, start_time: testDate }, + }, + }); }); it('builds a daily cron trigger with specified end date/time', () => { @@ -310,10 +333,12 @@ describe('Trigger', () => { (tree.instance() as Trigger).handleChange('intervalCategory')({ target: { value: PeriodicInterval.DAY }, }); - expect(spy).toHaveBeenLastCalledWith( - { cron_schedule: { cron: '0 0 0 * * ?', end_time: testDate, start_time: undefined } }, - '10', - ); + expect(spy).toHaveBeenLastCalledWith({ + ...PARAMS_DEFAULT, + trigger: { + cron_schedule: { ...CRON_DEFAULT, end_time: testDate, cron: '0 0 0 * * ?' }, + }, + }); }); it('builds a weekly cron trigger that runs every Monday, Friday, and Saturday', () => { @@ -327,10 +352,12 @@ describe('Trigger', () => { (tree.instance() as any)._toggleDay(1); (tree.instance() as any)._toggleDay(5); (tree.instance() as any)._toggleDay(6); - expect(spy).toHaveBeenLastCalledWith( - { cron_schedule: { cron: '0 0 0 ? * 1,5,6', end_time: undefined, start_time: undefined } }, - '10', - ); + expect(spy).toHaveBeenLastCalledWith({ + ...PARAMS_DEFAULT, + trigger: { + cron_schedule: { ...CRON_DEFAULT, cron: '0 0 0 ? * 1,5,6' }, + }, + }); }); it('builds a cron with the manually specified cron string, even if days are toggled', () => { @@ -350,16 +377,12 @@ describe('Trigger', () => { (tree.instance() as Trigger).handleChange('cron')({ target: { value: 'oops this will break!' }, }); - expect(spy).toHaveBeenLastCalledWith( - { - cron_schedule: { - cron: 'oops this will break!', - end_time: undefined, - start_time: undefined, - }, + expect(spy).toHaveBeenLastCalledWith({ + ...PARAMS_DEFAULT, + trigger: { + cron_schedule: { ...CRON_DEFAULT, cron: 'oops this will break!' }, }, - '10', - ); + }); }); }); }); diff --git a/frontend/src/components/Trigger.tsx b/frontend/src/components/Trigger.tsx index fd5bb83bb81..132a0f9aea8 100644 --- a/frontend/src/components/Trigger.tsx +++ b/frontend/src/components/Trigger.tsx @@ -80,6 +80,7 @@ export default class Trigger extends React.Component const [endDate, endTime] = dateToPickerFormat(inAWeek); return { + catchup: true, cron: '', editCron: false, endDate, @@ -93,7 +94,6 @@ export default class Trigger extends React.Component startDate, startTime, type: TriggerType.INTERVALED, - catchup: true, }; })(); @@ -401,9 +401,9 @@ export default class Trigger extends React.Component if (this.props.onChange) { this.props.onChange({ - trigger, - maxConcurrentRuns: trigger ? this.state.maxConcurrentRuns : undefined, catchup, + maxConcurrentRuns: trigger ? this.state.maxConcurrentRuns : undefined, + trigger, }); } }, diff --git a/frontend/src/components/__snapshots__/Trigger.test.tsx.snap b/frontend/src/components/__snapshots__/Trigger.test.tsx.snap index 44768588860..d2927fa2b9f 100644 --- a/frontend/src/components/__snapshots__/Trigger.test.tsx.snap +++ b/frontend/src/components/__snapshots__/Trigger.test.tsx.snap @@ -145,6 +145,35 @@ exports[`Trigger enables a single day on click 1`] = ` width={120} /> + + + } + label="Catchup" + /> + +

+ Whether the recurring run should catch up if behind schedule. Defaults to true. +

+

+ For example, if the recurring run is paused for a while and re-enabled afterwards. If catchup=true, the scheduler will catch up on (backfill) each missed interval. Otherwise, it only schedules the latest interval. +

+

+ Usually, if your pipeline handles backfill internally, you should turn catchup off to avoid duplicate work. +

+ + } + /> + @@ -457,6 +486,35 @@ exports[`Trigger renders all week days enabled 1`] = ` width={120} /> + + + } + label="Catchup" + /> + +

+ Whether the recurring run should catch up if behind schedule. Defaults to true. +

+

+ For example, if the recurring run is paused for a while and re-enabled afterwards. If catchup=true, the scheduler will catch up on (backfill) each missed interval. Otherwise, it only schedules the latest interval. +

+

+ Usually, if your pipeline handles backfill internally, you should turn catchup off to avoid duplicate work. +

+ + } + /> + @@ -769,6 +827,35 @@ exports[`Trigger renders periodic schedule controls for initial render 1`] = ` width={120} /> + + + } + label="Catchup" + /> + +

+ Whether the recurring run should catch up if behind schedule. Defaults to true. +

+

+ For example, if the recurring run is paused for a while and re-enabled afterwards. If catchup=true, the scheduler will catch up on (backfill) each missed interval. Otherwise, it only schedules the latest interval. +

+

+ Usually, if your pipeline handles backfill internally, you should turn catchup off to avoid duplicate work. +

+ + } + /> + @@ -979,6 +1066,35 @@ exports[`Trigger renders periodic schedule controls if the trigger type is CRON width={120} /> + + + } + label="Catchup" + /> + +

+ Whether the recurring run should catch up if behind schedule. Defaults to true. +

+

+ For example, if the recurring run is paused for a while and re-enabled afterwards. If catchup=true, the scheduler will catch up on (backfill) each missed interval. Otherwise, it only schedules the latest interval. +

+

+ Usually, if your pipeline handles backfill internally, you should turn catchup off to avoid duplicate work. +

+ + } + /> + @@ -1212,6 +1328,35 @@ exports[`Trigger renders week days if the trigger type is CRON and interval is w width={120} /> + + + } + label="Catchup" + /> + +

+ Whether the recurring run should catch up if behind schedule. Defaults to true. +

+

+ For example, if the recurring run is paused for a while and re-enabled afterwards. If catchup=true, the scheduler will catch up on (backfill) each missed interval. Otherwise, it only schedules the latest interval. +

+

+ Usually, if your pipeline handles backfill internally, you should turn catchup off to avoid duplicate work. +

+ + } + /> + diff --git a/frontend/src/lib/Apis.ts b/frontend/src/lib/Apis.ts index c37713b9d63..cacde425632 100644 --- a/frontend/src/lib/Apis.ts +++ b/frontend/src/lib/Apis.ts @@ -13,7 +13,7 @@ // limitations under the License. import * as portableFetch from 'portable-fetch'; -import { HTMLViewerConfig } from 'src/components/viewers/HTMLViewer'; +import { HTMLViewerConfig } from '../components/viewers/HTMLViewer'; import { ExperimentServiceApi, FetchAPI } from '../apis/experiment'; import { JobServiceApi } from '../apis/job'; import { ApiPipeline, PipelineServiceApi, ApiPipelineVersion } from '../apis/pipeline'; diff --git a/frontend/src/lib/RunUtils.ts b/frontend/src/lib/RunUtils.ts index b4183a6228e..675d3cb45d0 100644 --- a/frontend/src/lib/RunUtils.ts +++ b/frontend/src/lib/RunUtils.ts @@ -15,7 +15,7 @@ */ import { orderBy } from 'lodash'; -import { ApiParameter, ApiPipelineVersion } from 'src/apis/pipeline'; +import { ApiParameter, ApiPipelineVersion } from '../apis/pipeline'; import { Workflow } from 'third_party/argo-ui/argo_template'; import { ApiJob } from '../apis/job'; import { diff --git a/frontend/src/pages/GettingStarted.test.tsx b/frontend/src/pages/GettingStarted.test.tsx index bfbf1084481..5d8c624519a 100644 --- a/frontend/src/pages/GettingStarted.test.tsx +++ b/frontend/src/pages/GettingStarted.test.tsx @@ -4,7 +4,7 @@ import TestUtils, { formatHTML } from '../TestUtils'; import { render } from '@testing-library/react'; import { PageProps } from './Page'; import { Apis } from '../lib/Apis'; -import { ApiListPipelinesResponse } from 'src/apis/pipeline/api'; +import { ApiListPipelinesResponse } from '../apis/pipeline/api'; import snapshotDiff from 'snapshot-diff'; const PATH_BACKEND_CONFIG = '../../../backend/src/apiserver/config/sample_config.json'; diff --git a/frontend/src/pages/NewRun.test.tsx b/frontend/src/pages/NewRun.test.tsx index 2239625d809..564af541493 100644 --- a/frontend/src/pages/NewRun.test.tsx +++ b/frontend/src/pages/NewRun.test.tsx @@ -1649,6 +1649,7 @@ describe('NewRun', () => { enabled: true, max_concurrency: '10', name: 'test run name', + no_catchup: false, pipeline_spec: { parameters: MOCK_PIPELINE.parameters, }, diff --git a/frontend/src/pages/NewRun.tsx b/frontend/src/pages/NewRun.tsx index 4e2c16c7618..74388b83d3e 100644 --- a/frontend/src/pages/NewRun.tsx +++ b/frontend/src/pages/NewRun.tsx @@ -113,6 +113,27 @@ class NewRun extends Page<{}, NewRunState> { static contextType = NamespaceContext; context!: React.ContextType; + public state: NewRunState = { + catchup: true, + description: '', + errorMessage: '', + experimentName: '', + experimentSelectorOpen: false, + isBeingStarted: false, + isClone: false, + isFirstRunInExperiment: false, + isRecurringRun: false, + parameters: [], + pipelineName: '', + pipelineSelectorOpen: false, + pipelineVersionName: '', + pipelineVersionSelectorOpen: false, + runName: '', + uploadDialogOpen: false, + usePipelineFromRunLabel: 'Using pipeline from cloned run', + useWorkflowFromRun: false, + }; + private pipelineSelectorColumns = [ { customRenderer: NameWithTooltip, @@ -143,27 +164,6 @@ class NewRun extends Page<{}, NewRunState> { { label: 'Created at', flex: 1, sortKey: ExperimentSortKeys.CREATED_AT }, ]; - public state: NewRunState = { - description: '', - errorMessage: '', - experimentName: '', - experimentSelectorOpen: false, - isBeingStarted: false, - isClone: false, - isFirstRunInExperiment: false, - isRecurringRun: false, - parameters: [], - pipelineName: '', - pipelineSelectorOpen: false, - pipelineVersionName: '', - pipelineVersionSelectorOpen: false, - runName: '', - uploadDialogOpen: false, - usePipelineFromRunLabel: 'Using pipeline from cloned run', - useWorkflowFromRun: false, - catchup: true, - }; - public getInitialToolbarState(): ToolbarProps { return { actions: {}, @@ -508,9 +508,9 @@ class NewRun extends Page<{}, NewRunState> { onChange={({ trigger, maxConcurrentRuns, catchup }) => this.setStateSafe( { + catchup, maxConcurrentRuns, trigger, - catchup, }, this._validate.bind(this), ) @@ -1035,8 +1035,8 @@ class NewRun extends Page<{}, NewRunState> { newRun = Object.assign(newRun, { enabled: true, max_concurrency: this.state.maxConcurrentRuns || '1', - trigger: this.state.trigger, no_catchup: !this.state.catchup, + trigger: this.state.trigger, }); } diff --git a/frontend/src/pages/RecurringRunDetails.test.tsx b/frontend/src/pages/RecurringRunDetails.test.tsx index 4c3761894a6..f0e834bab50 100644 --- a/frontend/src/pages/RecurringRunDetails.test.tsx +++ b/frontend/src/pages/RecurringRunDetails.test.tsx @@ -66,6 +66,7 @@ describe('RecurringRunDetails', () => { enabled: true, id: 'test-job-id', max_concurrency: '50', + no_catchup: true, name: 'test job', pipeline_spec: { parameters: [{ name: 'param1', value: 'value1' }], @@ -80,21 +81,12 @@ describe('RecurringRunDetails', () => { }, } as ApiJob; - historyPushSpy.mockClear(); - updateBannerSpy.mockClear(); - updateDialogSpy.mockClear(); - updateSnackbarSpy.mockClear(); - updateToolbarSpy.mockClear(); + jest.clearAllMocks(); getJobSpy.mockImplementation(() => fullTestJob); - getJobSpy.mockClear(); - deleteJobSpy.mockClear(); deleteJobSpy.mockImplementation(); - enableJobSpy.mockClear(); enableJobSpy.mockImplementation(); - disableJobSpy.mockClear(); disableJobSpy.mockImplementation(); getExperimentSpy.mockImplementation(); - getExperimentSpy.mockClear(); }); afterEach(() => tree.unmount()); @@ -106,13 +98,18 @@ describe('RecurringRunDetails', () => { }); it('renders a recurring run with cron schedule', async () => { - fullTestJob.trigger = { - cron_schedule: { - cron: '* * * 0 0 !', - end_time: new Date(2018, 10, 9, 8, 7, 6), - start_time: new Date(2018, 9, 8, 7, 6), + const cronTestJob = { + ...fullTestJob, + no_catchup: undefined, // in api requests, it's undefined when false + trigger: { + cron_schedule: { + cron: '* * * 0 0 !', + end_time: new Date(2018, 10, 9, 8, 7, 6), + start_time: new Date(2018, 9, 8, 7, 6), + }, }, }; + getJobSpy.mockImplementation(() => cronTestJob); tree = shallow(); await TestUtils.flushPromises(); expect(tree).toMatchSnapshot(); diff --git a/frontend/src/pages/RecurringRunDetails.tsx b/frontend/src/pages/RecurringRunDetails.tsx index 3144ab07f92..d6e005f41f9 100644 --- a/frontend/src/pages/RecurringRunDetails.tsx +++ b/frontend/src/pages/RecurringRunDetails.tsx @@ -85,7 +85,7 @@ class RecurringRunDetails extends Page<{}, RecurringRunConfigState> { if (run.max_concurrency) { triggerDetails.push(['Max. concurrent runs', run.max_concurrency]); } - triggerDetails.push(['Catchup', `${run.no_catchup || false}`]); + triggerDetails.push(['Catchup', `${!run.no_catchup}`]); if (run.trigger.cron_schedule && run.trigger.cron_schedule.start_time) { triggerDetails.push([ 'Start time', diff --git a/frontend/src/pages/__snapshots__/RecurringRunDetails.test.tsx.snap b/frontend/src/pages/__snapshots__/RecurringRunDetails.test.tsx.snap index 25043260e63..c8ad38cc0b9 100644 --- a/frontend/src/pages/__snapshots__/RecurringRunDetails.test.tsx.snap +++ b/frontend/src/pages/__snapshots__/RecurringRunDetails.test.tsx.snap @@ -37,6 +37,10 @@ exports[`RecurringRunDetails renders a recurring run with cron schedule 1`] = ` "Max. concurrent runs", "50", ], + Array [ + "Catchup", + "true", + ], Array [ "Start time", "10/8/2018, 7:06:00 AM", @@ -101,6 +105,10 @@ exports[`RecurringRunDetails renders a recurring run with periodic schedule 1`] "Max. concurrent runs", "50", ], + Array [ + "Catchup", + "false", + ], Array [ "Start time", "10/8/2018, 7:06:00 AM",