feat(scheduler): flexible time windows#28098
Conversation
…feat-scheduler-flex-window
There was a problem hiding this comment.
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. |
go-to-k
left a comment
There was a problem hiding this comment.
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.
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.
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.
| 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(), | ||
| }; | ||
| } |
There was a problem hiding this comment.
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(), | |
| }; | |
| } |
| }).toThrow('maximumWindowInMinutes must be provided when flexibleTimeWindowMode is set to FLEXIBLE'); | ||
| }); | ||
|
|
||
| test('throw error when maximumWindowInMinutes is more than 1440', () => { |
There was a problem hiding this comment.
It is not big deal but...
| test('throw error when maximumWindowInMinutes is more than 1440', () => { | |
| test('throw error when maximumWindowInMinutes is greater than 1440', () => { |
| 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(), | ||
| }; | ||
| } |
There was a problem hiding this comment.
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(), | |
| }; | |
| } |
|
@go-to-k |
scanlonp
left a comment
There was a problem hiding this comment.
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.
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 |
kaizencc
left a comment
There was a problem hiding this comment.
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.
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.
| 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.
| 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.
| export class FlexibleTimeWindowMode { | |
| export class TimeWindow { |
There was a problem hiding this comment.
Will be nicer to write TimeWindow.flexible()
| * | ||
| * @default - Required if mode is FLEXIBLE. | ||
| */ | ||
| public readonly maximumWindowInMinutes?: Duration; |
There was a problem hiding this comment.
| public readonly maximumWindowInMinutes?: Duration; | |
| public readonly maxWindow?: Duration; |
| * | ||
| * @default FlexibleTimeWindowMode.off() | ||
| */ | ||
| readonly flexibleTimeWindow?: FlexibleTimeWindowMode; |
There was a problem hiding this comment.
| readonly flexibleTimeWindow?: FlexibleTimeWindowMode; | |
| readonly timeWindow?: TimeWindow; |
…feat-scheduler-flex-window
Pull request has been modified.
|
@kaizencc |
kaizencc
left a comment
There was a problem hiding this comment.
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
flexibleTimeWindowfeature 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
SchedulePropsinterfaceflexibleTimeWindowModeproperty defaults toOFFto avoid a breaking change.set the added property to
CfnScheduleconstructBasically, just set the values as documented, but with the following validations.
flexibleTimeWindowModeisFLEXIBLEmaximumWindowInMinutesmust be specifiedmaximumWindowInMinutesmust 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=>customizableBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license