-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(scheduler): flexible time windows #28098
Conversation
…feat-scheduler-flex-window
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR!
I would like to add a new interface for FlexibleTimeWindow
, what do you think?
Even if there are more properties associated with FlexibleTimeWindow
in the future, it might not be too complicated. It might also be simpler than validation by annotation (as it is now).
@go-to-k I'm wondering if the addition of the new interface is in line with this guideline. |
Thanks, I see. Certainly correct. Let's go with this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commented some points. Please feel free to ask me if you need anything.
/** | ||
* FlexibleTimeWindow is disabled. | ||
*/ | ||
OFF = 'OFF', | ||
/** | ||
* FlexibleTimeWindow is enabled. | ||
*/ | ||
FLEXIBLE = 'FLEXIBLE' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the meantime, please put in a line break.
/** | |
* FlexibleTimeWindow is disabled. | |
*/ | |
OFF = 'OFF', | |
/** | |
* FlexibleTimeWindow is enabled. | |
*/ | |
FLEXIBLE = 'FLEXIBLE' | |
/** | |
* FlexibleTimeWindow is disabled. | |
*/ | |
OFF = 'OFF', | |
/** | |
* FlexibleTimeWindow is enabled. | |
*/ | |
FLEXIBLE = 'FLEXIBLE' |
/** | ||
* The maximum time window during which the schedule can be invoked. | ||
* | ||
* @default - Required if flexibleTimeWindowMode is FLEXIBLE. | ||
*/ | ||
readonly maximumWindowInMinutes?: Duration; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maximumWindowInMinutes must be set from 1 to 1440 minutes
I think it's user friendly to mention a range of the value that can be specified in this doc.
flexibleTimeWindowMode?: FlexibleTimeWindowMode, maximumWindowInMinutes?: Duration, | ||
): CfnSchedule.FlexibleTimeWindowProperty { | ||
if (!flexibleTimeWindowMode || flexibleTimeWindowMode === FlexibleTimeWindowMode.OFF) { | ||
if (maximumWindowInMinutes) { | ||
Annotations.of(this).addWarningV2('@aws-cdk/aws-scheduler:maximumWindowInMinutesIgnored', 'maximumWindowInMinutes is ignored when flexibleTimeWindowMode is not FLEXIBLE'); | ||
} | ||
return { | ||
mode: 'OFF', | ||
}; | ||
} | ||
|
||
if (!maximumWindowInMinutes) { | ||
throw new Error('maximumWindowInMinutes must be provided when flexibleTimeWindowMode is set to FLEXIBLE'); | ||
} | ||
if (maximumWindowInMinutes.toMinutes() < 1 || maximumWindowInMinutes.toMinutes() > 1440) { | ||
throw new Error(`maximumWindowInMinutes must be between 1 and 1440, got ${maximumWindowInMinutes.toMinutes()}`); | ||
} | ||
return { | ||
mode: 'FLEXIBLE', | ||
maximumWindowInMinutes: maximumWindowInMinutes.toMinutes(), | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion,
- Since this is not a case of using deprecated properties, affecting core params for the construct, etc., I do not think it is a case that is harmful enough to the user to warn using annotations.
- I think it would be more consistent to use enum.
Feel free to comment.
private renderFlexibleTimeWindow( | |
flexibleTimeWindowMode?: FlexibleTimeWindowMode, maximumWindowInMinutes?: Duration, | |
): CfnSchedule.FlexibleTimeWindowProperty { | |
if (!flexibleTimeWindowMode || flexibleTimeWindowMode === FlexibleTimeWindowMode.OFF) { | |
if (maximumWindowInMinutes) { | |
Annotations.of(this).addWarningV2('@aws-cdk/aws-scheduler:maximumWindowInMinutesIgnored', 'maximumWindowInMinutes is ignored when flexibleTimeWindowMode is not FLEXIBLE'); | |
} | |
return { | |
mode: 'OFF', | |
}; | |
} | |
if (!maximumWindowInMinutes) { | |
throw new Error('maximumWindowInMinutes must be provided when flexibleTimeWindowMode is set to FLEXIBLE'); | |
} | |
if (maximumWindowInMinutes.toMinutes() < 1 || maximumWindowInMinutes.toMinutes() > 1440) { | |
throw new Error(`maximumWindowInMinutes must be between 1 and 1440, got ${maximumWindowInMinutes.toMinutes()}`); | |
} | |
return { | |
mode: 'FLEXIBLE', | |
maximumWindowInMinutes: maximumWindowInMinutes.toMinutes(), | |
}; | |
} | |
private renderFlexibleTimeWindow( | |
flexibleTimeWindowMode?: FlexibleTimeWindowMode, maximumWindowInMinutes?: Duration, | |
): CfnSchedule.FlexibleTimeWindowProperty { | |
if (!flexibleTimeWindowMode || flexibleTimeWindowMode === FlexibleTimeWindowMode.OFF) { | |
return { | |
mode: FlexibleTimeWindowMode.OFF, | |
}; | |
} | |
if (!maximumWindowInMinutes) { | |
throw new Error('maximumWindowInMinutes must be provided when flexibleTimeWindowMode is set to FLEXIBLE'); | |
} | |
if (maximumWindowInMinutes.toMinutes() < 1 || maximumWindowInMinutes.toMinutes() > 1440) { | |
throw new Error(`maximumWindowInMinutes must be between 1 and 1440, got ${maximumWindowInMinutes.toMinutes()}`); | |
} | |
return { | |
mode: flexibleTimeWindowMode, | |
maximumWindowInMinutes: maximumWindowInMinutes.toMinutes(), | |
}; | |
} |
}); | ||
|
||
test('throw error when maximumWindowInMinutes is more than 1440', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not big deal but...
test('throw error when maximumWindowInMinutes is more than 1440', () => { | |
test('throw error when maximumWindowInMinutes is greater than 1440', () => { |
flexibleTimeWindowMode?: FlexibleTimeWindowMode, maximumWindowInMinutes?: Duration, | ||
): CfnSchedule.FlexibleTimeWindowProperty { | ||
if (!flexibleTimeWindowMode || flexibleTimeWindowMode === FlexibleTimeWindowMode.OFF) { | ||
return { | ||
mode: FlexibleTimeWindowMode.OFF, | ||
}; | ||
} | ||
|
||
if (!maximumWindowInMinutes) { | ||
throw new Error('maximumWindowInMinutes must be provided when flexibleTimeWindowMode is set to FLEXIBLE'); | ||
} | ||
if (maximumWindowInMinutes.toMinutes() < 1 || maximumWindowInMinutes.toMinutes() > 1440) { | ||
throw new Error(`maximumWindowInMinutes must be between 1 and 1440, got ${maximumWindowInMinutes.toMinutes()}`); | ||
} | ||
return { | ||
mode: flexibleTimeWindowMode, | ||
maximumWindowInMinutes: maximumWindowInMinutes.toMinutes(), | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
Sorry, this one might be better. Because this one has fewer branching conditions. (The actual quantity will not change, but...)
private renderFlexibleTimeWindow( | |
flexibleTimeWindowMode?: FlexibleTimeWindowMode, maximumWindowInMinutes?: Duration, | |
): CfnSchedule.FlexibleTimeWindowProperty { | |
if (!flexibleTimeWindowMode || flexibleTimeWindowMode === FlexibleTimeWindowMode.OFF) { | |
return { | |
mode: FlexibleTimeWindowMode.OFF, | |
}; | |
} | |
if (!maximumWindowInMinutes) { | |
throw new Error('maximumWindowInMinutes must be provided when flexibleTimeWindowMode is set to FLEXIBLE'); | |
} | |
if (maximumWindowInMinutes.toMinutes() < 1 || maximumWindowInMinutes.toMinutes() > 1440) { | |
throw new Error(`maximumWindowInMinutes must be between 1 and 1440, got ${maximumWindowInMinutes.toMinutes()}`); | |
} | |
return { | |
mode: flexibleTimeWindowMode, | |
maximumWindowInMinutes: maximumWindowInMinutes.toMinutes(), | |
}; | |
} | |
private renderFlexibleTimeWindow( | |
flexibleTimeWindowMode?: FlexibleTimeWindowMode, maximumWindowInMinutes?: Duration, | |
): CfnSchedule.FlexibleTimeWindowProperty { | |
const mode = flexibleTimeWindowMode ?? FlexibleTimeWindowMode.OFF; | |
if (mode === FlexibleTimeWindowMode.OFF) { | |
return { | |
mode, | |
}; | |
} | |
if (!maximumWindowInMinutes) { | |
throw new Error('maximumWindowInMinutes must be provided when flexibleTimeWindowMode is set to FLEXIBLE'); | |
} | |
if (maximumWindowInMinutes.toMinutes() < 1 || maximumWindowInMinutes.toMinutes() > 1440) { | |
throw new Error(`maximumWindowInMinutes must be between 1 and 1440, got ${maximumWindowInMinutes.toMinutes()}`); | |
} | |
return { | |
mode, | |
maximumWindowInMinutes: maximumWindowInMinutes.toMinutes(), | |
}; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good.
@go-to-k |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @sakurai-ryo, apologies we are getting around to this a little late. I think this case is a great opportunity for an Enum like class! For example
new scheduler.Schedule(stack, 'UseFlexibleTimeWindow', {
schedule: expression,
target: target,
flexibleTimeWindow: scheduler.FlexibleTimeWindowMode.Flexible(cdk.Duration.minutes(10)),
});
and
new scheduler.Schedule(stack, 'UseFlexibleTimeWindow', {
schedule: expression,
target: target,
flexibleTimeWindow: scheduler.FlexibleTimeWindowMode.Off(),
});
This will tightly couple the mode to the properties it needs! I think this will give the best user experience and be extensible.
You can certainly improve my naming as well!
…feat-scheduler-flex-window
…feat-scheduler-flex-window
Pull request has been modified.
/** | ||
* FlexibleTimeWindow is disabled. | ||
*/ | ||
public static off(): FlexibleTimeWindowMode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to camelCase because it resulted in an error during cdk-build.
error JSII8002: Method and property (unless they are static readonly) names must use camelCase. Rename "@aws-cdk/aws-scheduler-alpha.FlexibleTimeWindowMode.Flexible" to "flexible"
error JSII8002: Method and property (unless they are static readonly) names must use camelCase. Rename "@aws-cdk/aws-scheduler-alpha.FlexibleTimeWindowMode.Off" to "off"
@scanlonp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sakurai-ryo, I have some naming convention nits here. But otherwise looks good
* | ||
* Must be between 1 to 1440 minutes. | ||
* | ||
* @default - Required if mode is FLEXIBLE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't the correct use of the @default
tag. Should describe the default if mode is not flexible
/** | ||
* Determines whether the schedule is invoked within a flexible time window. | ||
*/ | ||
public readonly mode: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public readonly mode: string; | |
public readonly mode: 'OFF' | 'FLEXIBLE'; |
/** | ||
* FlexibleTimeWindow is enabled. | ||
*/ | ||
public static flexible(maximumWindowInMinutes: Duration): FlexibleTimeWindowMode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static flexible(maximumWindowInMinutes: Duration): FlexibleTimeWindowMode { | |
public static flexible(maxWindow: Duration): FlexibleTimeWindowMode { |
It doesn't need to be in minutes anymore cuz we're supplying a Duration
/** | ||
* A time window during which EventBridge Scheduler invokes the schedule. | ||
*/ | ||
export class FlexibleTimeWindowMode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export class FlexibleTimeWindowMode { | |
export class TimeWindow { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be nicer to write TimeWindow.flexible()
* | ||
* @default - Required if mode is FLEXIBLE. | ||
*/ | ||
public readonly maximumWindowInMinutes?: Duration; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public readonly maximumWindowInMinutes?: Duration; | |
public readonly maxWindow?: Duration; |
* | ||
* @default FlexibleTimeWindowMode.off() | ||
*/ | ||
readonly flexibleTimeWindow?: FlexibleTimeWindowMode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readonly flexibleTimeWindow?: FlexibleTimeWindowMode; | |
readonly timeWindow?: TimeWindow; |
…feat-scheduler-flex-window
Pull request has been modified.
@kaizencc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh shoot! Merge conflicts with your other PR that got merged :). Do you mind fixing those conflicts @sakurai-ryo? Everything else looks good.
…feat-scheduler-flex-window
Pull request has been modified.
@kaizencc |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
This PR adds support for configuring flexible time windows.
Description
Currently, users cannot configure the
flexibleTimeWindow
feature in the Scheduler construct.This feature enhances flexibility and reliability, allowing tasks to be invoked within a defined time window.
https://docs.aws.amazon.com/scheduler/latest/UserGuide/managing-schedule-flexible-time-windows.html
CloudFormation allows users to take advantage of this feature as follows.
With this template, it will invokes the target within 10 minutes after the scheduled time.
Changes
add Enum indicating flexible time window mode
Currently there are only two modes, FLEXIBLE and OFF, so there is no problem using boolean instead of enum.
But I think it's better to use Enum to prepare for future expansion.
https://docs.aws.amazon.com/ja_jp/AWSCloudFormation/latest/UserGuide/aws-properties-scheduler-schedule-flexibletimewindow.html
add property to
ScheduleProps
interfaceflexibleTimeWindowMode
property defaults toOFF
to avoid a breaking change.set the added property to
CfnSchedule
constructBasically, just set the values as documented, but with the following validations.
flexibleTimeWindowMode
isFLEXIBLE
maximumWindowInMinutes
must be specifiedmaximumWindowInMinutes
must be set from 1 to 1440 minuteshttps://docs.aws.amazon.com/ja_jp/AWSCloudFormation/latest/UserGuide/aws-properties-scheduler-schedule-flexibletimewindow.html
In addition, I added some unit tests and integ-tests.
others
customizeable
=>customizable
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license