Skip to content
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

Merged
merged 20 commits into from Dec 15, 2023

Conversation

sakurai-ryo
Copy link
Contributor

@sakurai-ryo sakurai-ryo commented Nov 22, 2023

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.

Resources:
  Schedule:
    Type: AWS::Scheduler::Schedule
    Properties: 
      FlexibleTimeWindow: 
        Mode: "FLEXIBLE" # or "OFF"
        MaximumWindowInMinutes: 10 # between 1 and 1440
      Name: "sample-schedule"
      ScheduleExpression: "cron(0 9 * * ? *)"
      State: "ENABLED"
      Target:
        Arn: hoge
        RoleArn: hoge

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 interface

flexibleTimeWindowMode property defaults to OFF to avoid a breaking change.

interface ScheduleProps {
  // ....
  /**
   * Determines whether the schedule is invoked within a flexible time window.
   *
   * @see https://docs.aws.amazon.com/scheduler/latest/UserGuide/managing-schedule-flexible-time-windows.html
   *
   * @default - FlexibleTimeWindowMode.OFF
   */
  readonly flexibleTimeWindowMode?: FlexibleTimeWindowMode;

  /**
   * The maximum time window during which the schedule can be invoked.
   *
   * @default - Required if flexibleTimeWindowMode is FLEXIBLE.
   */
  readonly maximumWindowInMinutes?: Duration;
}

set the added property to CfnSchedule construct

Basically, just set the values as documented, but with the following validations.

  • If flexibleTimeWindowMode is FLEXIBLE
    • maximumWindowInMinutes must be specified
    • maximumWindowInMinutes must be set from 1 to 1440 minutes

https://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

  • fixed typo in README
    • customizeable => customizable

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added p2 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK labels Nov 22, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team November 22, 2023 04:48
@sakurai-ryo sakurai-ryo changed the title feat(scheduler): add support for configuring flexible time windows feat(scheduler): add support for flexible time windows Nov 22, 2023
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 22, 2023
Copy link
Contributor

@go-to-k go-to-k left a 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).

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 22, 2023
@sakurai-ryo
Copy link
Contributor Author

@go-to-k
Thank you for your feedback!
Initially, I had implemented this by defining a new interface, but after reviewing the section outlined in the DESIGN_GUIDELINES, I decided to go with the current implementation (although the naming of the properties is somewhat confusing).
https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md#flat

I'm wondering if the addition of the new interface is in line with this guideline.

@go-to-k
Copy link
Contributor

go-to-k commented Nov 22, 2023

Thanks, I see. Certainly correct. Let's go with this.
Please wait a moment so I can look at the other parts.

Copy link
Contributor

@go-to-k go-to-k left a 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.

Comment on lines 66 to 73
/**
* FlexibleTimeWindow is disabled.
*/
OFF = 'OFF',
/**
* FlexibleTimeWindow is enabled.
*/
FLEXIBLE = 'FLEXIBLE'
Copy link
Contributor

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.

Suggested change
/**
* FlexibleTimeWindow is disabled.
*/
OFF = 'OFF',
/**
* FlexibleTimeWindow is enabled.
*/
FLEXIBLE = 'FLEXIBLE'
/**
* FlexibleTimeWindow is disabled.
*/
OFF = 'OFF',
/**
* FlexibleTimeWindow is enabled.
*/
FLEXIBLE = 'FLEXIBLE'

Comment on lines 142 to 147
/**
* The maximum time window during which the schedule can be invoked.
*
* @default - Required if flexibleTimeWindowMode is FLEXIBLE.
*/
readonly maximumWindowInMinutes?: Duration;
Copy link
Contributor

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.

Comment on lines 341 to 363
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(),
};
}
Copy link
Contributor

@go-to-k go-to-k Nov 22, 2023

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.

Suggested change
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', () => {
Copy link
Contributor

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...

Suggested change
test('throw error when maximumWindowInMinutes is more than 1440', () => {
test('throw error when maximumWindowInMinutes is greater than 1440', () => {

Comment on lines 344 to 363
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(),
};
}
Copy link
Contributor

@go-to-k go-to-k Nov 24, 2023

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...)

Suggested change
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(),
};
}

Copy link
Contributor

@go-to-k go-to-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good.

@sakurai-ryo
Copy link
Contributor Author

@go-to-k
Thanks for the quick review!

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 24, 2023
Copy link
Contributor

@scanlonp scanlonp left a 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!

@scanlonp scanlonp self-assigned this Dec 5, 2023
@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 5, 2023
@mergify mergify bot dismissed scanlonp’s stale review December 8, 2023 12:05

Pull request has been modified.

/**
* FlexibleTimeWindow is disabled.
*/
public static off(): FlexibleTimeWindowMode {
Copy link
Contributor Author

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"

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 8, 2023
@sakurai-ryo
Copy link
Contributor Author

@scanlonp
No problem at all! Thank you for the review!
I've made the changes. Please review them again.

Copy link
Contributor

@kaizencc kaizencc left a 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.
Copy link
Contributor

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public readonly mode: string;
public readonly mode: 'OFF' | 'FLEXIBLE';

/**
* FlexibleTimeWindow is enabled.
*/
public static flexible(maximumWindowInMinutes: Duration): FlexibleTimeWindowMode {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export class FlexibleTimeWindowMode {
export class TimeWindow {

Copy link
Contributor

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public readonly maximumWindowInMinutes?: Duration;
public readonly maxWindow?: Duration;

*
* @default FlexibleTimeWindowMode.off()
*/
readonly flexibleTimeWindow?: FlexibleTimeWindowMode;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonly flexibleTimeWindow?: FlexibleTimeWindowMode;
readonly timeWindow?: TimeWindow;

@kaizencc kaizencc changed the title feat(scheduler): add support for flexible time windows feat(scheduler): flexible time windows Dec 12, 2023
@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 12, 2023
@mergify mergify bot dismissed kaizencc’s stale review December 13, 2023 12:30

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 13, 2023
@sakurai-ryo
Copy link
Contributor Author

@kaizencc
Thanks! I fixed them.

Copy link
Contributor

@kaizencc kaizencc left a 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.

@mergify mergify bot dismissed kaizencc’s stale review December 14, 2023 02:10

Pull request has been modified.

@sakurai-ryo
Copy link
Contributor Author

@kaizencc
fixed those conflicts 👍

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: fb820e9
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Copy link
Contributor

mergify bot commented Dec 15, 2023

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).

@mergify mergify bot merged commit 6554e48 into aws:main Dec 15, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2 pr/needs-maintainer-review This PR needs a review from a Core Team Member repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants