RFC 474: Event Bridge Scheduler L2

[filletofish]

This is a request for comments about EventBridge Scheduler L2 Construct. See #474 for
additional details.

APIs are signed off by @kaizencc

[kaizencc]

Hi @filletofish, @Jacco, starting off with a few comments on the proposed APIs. Nothing too major. I'll be back to look at the rest soon.

[kaizencc]

TimeZone enum is added to core: aws/aws-cdk#24127. We should use it.

[filletofish]

Amazing, thanks!

[kaizencc]

Hi! @filletofish @Jacco. I am back, and have just a few more comments on this RFC. Once they are addressed I will swiftly approve and we can get to the implementation. Thanks for all your work thus far!

Pull request has been modified.

[comcalvi]

are all three of these part of the retry policy?

[filletofish]

In CFN only maximum event age and maximum retry attempts are part of retry policy. Would you recommend to organise them similarly in a RetryPolicy structure as well?

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-scheduler-schedule-retrypolicy.html

Pull request has been modified.

[filletofish]

Thank you for your feedback @comcalvi! I have made some updates per your comments.

[tmokmss]

A few minor comments from one of the users' perspective. Looking forward to using this feature! 🚀

[tmokmss]

How about renaming the prop scheduleTimezone to timezone to make it short? I feel it is already clear enough that this timezone property is for the schedule.

[filletofish]

I am concerned that the customer might think that timezone is also applied to other properties. For example, Schedule also has properties startDate and endDate, which should be specified in UTC timezone and do not depend on timezone.

So we have these options:

1 - Do nothing

Keep interface as-is with a separate scheduleTimeZone parameter. It's simple and matches CFN structure https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-scheduler-schedule.html#cfn-scheduler-schedule-scheduleexpressiontimezone

2 - Rename scheduleTimeZone to timeZone

Not obvious that the property timeZone does not apply to startDate and endDate

3 - Add timezone parameter to ScheduleExpression methods

Example:

const cronBasedSchedule = new Schedule(this, 'Schedule', {
    schedule: ScheduleExpression.cron({ minute: '0', hour: '23', day: '20', month: '11', timezone: TimeZone.AMERICA_NEW_YORK}),
    target,
    description: 'This is a test cron-based schedule that will run at 11:00 PM, on day 20 of the month, only in November in New York timezone',
});

In implementation we will create a class ScheduleExpression that will inherit from events.Schedule and will have methods cron(..), at(..) and rate(..) with timezone as an additional parameter.

Not a bad option. Maybe timezone will not be that discoverable as in other options.

4 - Create a separate interface with two parameters expression and timezone

Example:

const cronBasedSchedule = new Schedule(this, 'Schedule', {
    scheduleExpression: {
          expression: ScheduleExpression.cron({ minute: '0', hour: '23', day: '20', month: '11' }),
          timeZone: TimeZone.AMERICA_NEW_YORK
     },
    target,
    description: 'This is a test cron-based schedule that will run at 11:00 PM, on day 20 of the month, only in November in New York timezone',
});

However, I am not sure what name to choose for the new interface as names Schedule and ScheduleExpression are already taken. Any recommendations?

---

All in all, I am probably still leaning to the first as it's the simplest option. Curious if you guys also have any opinion on this question @Jacco, @kaizencc and @comcalvi?

[tmokmss]

That makes sense, thanks! To me, option 3 or 4 look nice and more cohesive, but at the same time option 1 sounds reasonable. I'm fine with whatever decision is made :)

[kaizencc]

I think I like option 4, with scheduleExpression: { schedule: Schedule, timeZone: TimeZone }

[filletofish]

I think I like option 4, with scheduleExpression: { schedule: Schedule, timeZone: TimeZone }

But then there will be two classes with names Schedule. Can't find a better class names, do you have any ideas?

[Jacco]

Few things to consider:

How would the original events.Schedule be changed if timezone support is added for aws-events. I think it would best added CronOptions in CDK since it has no meaning for a rate expressions timeZone has no meaning. It would be kept separate though because in cloudFormation is would probably not be part of the expression but a separate property like here.

So maybe we should still used events.Schedule functionality but not inherit and add the timeZone parameter from cron and at.

Unflat interfaces give less good code completion in some languages (like python).

Another thing to consider: what if aws-events at some point adds another type of schedule that is unsupported by aws-scheduler. We could add an error raising override, but maybe we'd better not inherited?

Maybe implement like this:

import { Duration, TimeZone } from 'aws-cdk-lib';
import * as events from 'aws-cdk-lib/aws-events';

export abstract class ScheduleExpression {
  public static at(date: Date, timeZone?: TimeZone): ScheduleExpression {
    try {
      const literal = date.toISOString().split('.')[0];
      return new LiteralScheduleExpression(`at(${literal})`, timeZone)
    } catch(e) {
      if (e instanceof RangeError){
        throw new Error('Invalid date');
      }
      throw e;
    }
  }

  public static cron(options: events.CronOptions, timeZone?: TimeZone): ScheduleExpression {
    return new LiteralScheduleExpression(events.Schedule.cron(options).expressionString, timeZone);
  }

  public static rate(duration: Duration): ScheduleExpression {
    return new LiteralScheduleExpression(events.Schedule.rate(duration).expressionString);
  }

  public static expression(expression: string, timeZone?: TimeZone) {
    return new LiteralScheduleExpression(expression, timeZone);
  }

   public abstract readonly expressionString: string;

   public abstract readonly expressionTimeZone?: TimeZone;
}

class LiteralScheduleExpression extends ScheduleExpression {
  constructor(public readonly expressionString: string, public readonly expressionTimeZone?: TimeZone) {
    super();
  }

  public _bind() {}
}

[filletofish]

I like that suggestion @Jacco. It's similar to option 3. I was also thinking about the way to reuse functionality of events.Schedule with composition, but couldn't come up with a good enough option like yours.

I will add it to the RFC. Thanks!

[kaizencc]

Agree on not inheriting for now. The ideal scenario is still to put a generic Schedule into core that can be extended by the 4 resources that have the concept of schedule today. But that's out of scope of this RFC.

(autoscaling, app-autoscaling, scheduler, events are the 4 i can think of)

[comcalvi]

This is great work, I just have a few minor comments

[comcalvi]

Very nice!

[comcalvi]

I like this a lot. Why not move the timezone to the struct that's taken as the first argument?

[filletofish]

nice one, will add

Pull request has been modified.

Letting Kaizen approve

Pull request has been modified.