feat(lambda-event-sources): add AT_TIMESTAMP event source mapping starting position by cecheta · Pull Request #20741 · aws/aws-cdk

cecheta · 2022-06-14T21:31:59Z

Closes #17214

This PR adds AT_TIMESTAMP to the StartingPosition enum for lambda event source mappings.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-06-14T21:32:09Z

kaizencc

Hi @cecheta! Thanks for submitting this PR! I've provided some comments mostly surrounding the documentation around this feature. There's a lot of weird things going on, like the property only being allowed in Kinesis streams and being used in conjunction with AT_TIMESTAMP. So we need to be very careful with the docs.

kaizencc · 2022-07-05T16:37:43Z

packages/@aws-cdk/aws-lambda-event-sources/lib/kinesis.ts

+   *
+   * @default Required when startingPosition is AT_TIMESTAMP.
+   */
+  readonly startingPositionTimestamp?: number


Suggested change

readonly startingPositionTimestamp?: number

readonly startingPositionTimestamp?: number;

kaizencc · 2022-07-05T16:38:39Z

packages/@aws-cdk/aws-lambda-event-sources/lib/kinesis.ts

+  /**
+   * The time from which to start reading, in Unix time seconds.
+   *
+   * @default Required when startingPosition is AT_TIMESTAMP.


This is not the correct use of default. The @default tag is meant to describe what happens if left blank -- should be @default - no timestamp. The information about when it's required is important, but put that in the regular documentation instead.

kaizencc · 2022-07-05T16:39:23Z

packages/@aws-cdk/aws-lambda/lib/event-source-mapping.ts

+  /**
+   * The time from which to start reading, in Unix time seconds.
+   *
+   * @default Required when startingPosition is AT_TIMESTAMP.


same comment regarding this @default

i also see how @default is being used in startingPosition. I consider that wrong also. Would you please change that as well?

Updated both

kaizencc · 2022-07-05T16:42:40Z

packages/@aws-cdk/aws-lambda/lib/event-source-mapping.ts

  LATEST = 'LATEST',
+
+  /**
+   * Start reading from a position defined by a time stamp.


This documentation should mention that if supplied, startingPositionTimestamp must be set

kaizencc · 2022-07-05T16:43:52Z

packages/@aws-cdk/aws-lambda/lib/event-source-mapping.ts

+    if (props.startingPosition === StartingPosition.AT_TIMESTAMP && !props.startingPositionTimestamp) {
+      throw new Error('startingPositionTimestamp must be provided when startingPosition is AT_TIMESTAMP');
+    }


what about when startingPositionTimestamp is set but props.startingPosition !== StartingPosition.AT_TIMESTAMP? I think we should throw an error also

Added another error and a test

kaizencc · 2022-07-05T16:45:15Z

packages/@aws-cdk/aws-lambda/lib/event-source-mapping.ts

+
+  /**
+   * Start reading from a position defined by a time stamp.
+   * Only supported for Amazon Kinesis streams.


what happens when this is set for non Kinesis streams?

Updated the docs to say that an error will occur

kaizencc · 2022-07-05T16:46:23Z

packages/@aws-cdk/aws-lambda-event-sources/lib/kinesis.ts

 * Use an Amazon Kinesis stream as an event source for AWS Lambda.
 */
 export class KinesisEventSource extends StreamEventSource {
+  private innerProps: KinesisEventSourceProps;


I would prefer you not do this and just pull out the props you're going to use. Looks like just a startingPositionTimestamp is necessary.

kaizencc · 2022-07-05T16:46:55Z

packages/@aws-cdk/aws-lambda-event-sources/README.md

 * __retryAttempts__: The maximum number of times a record should be retried in the event of failure.
-* __startingPosition__: Will determine where to being consumption, either at the most recent ('LATEST') record or the oldest record ('TRIM_HORIZON'). 'TRIM_HORIZON' will ensure you process all available data, while 'LATEST' will ignore all records that arrived prior to attaching the event source.
+* __startingPosition__: Will determine where to being consumption. 'LATEST' will start at the most recent record and ignore all records that arrived prior to attaching the event source, 'TRIM_HORIZON' will start at the oldest record and ensure you process all available data, while 'AT_TIMESTAMP' will start reading records from a specified time stamp.
+* __startingPositionTimestamp__: The time stamp from which to start reading. Used in conjunction with __startingPosition__.


README needs to mention that AT_TIMESTAMP only works for kinesis streams.

Pull request has been modified.

mergify · 2022-07-21T13:01:52Z

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

aws-cdk-automation · 2022-07-21T13:38:44Z

AWS CodeBuild CI Report

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

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

mergify · 2022-07-21T13:39:01Z

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

…rting position (aws#20741) Closes aws#17214 This PR adds `AT_TIMESTAMP` to the `StartingPosition` enum for lambda event source mappings. ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [X] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws-cdk-automation requested a review from a team June 14, 2022 21:32

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Jun 14, 2022

kaizencc previously requested changes Jul 5, 2022

View reviewed changes

kaizencc changed the title ~~feat(lambda): add AT_TIMESTAMP event source mapping starting position~~ feat(lambda-event-sources): add AT_TIMESTAMP event source mapping starting position Jul 5, 2022

Chinedum Echeta and others added 4 commits July 9, 2022 21:33

feat(lambda): add AT_TIMESTAMP event source mapping starting position

b13b296

feat(lambda): Update README

8a50a08

feat(lambda): Add integration test

7cc7bd2

feat(lambda-event-sources): address review comments

8086e23

otaviomacedo approved these changes Jul 21, 2022

View reviewed changes

Merge branch 'main' into at-timestamp

925f311

mergify bot merged commit 76e0768 into aws:main Jul 21, 2022

	readonly startingPositionTimestamp?: number
	readonly startingPositionTimestamp?: number;

Conversation

cecheta commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Adding new Unconventional Dependencies:

New Features

Uh oh!

gitpod-io bot commented Jun 14, 2022

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jul 21, 2022

Uh oh!

aws-cdk-automation commented Jul 21, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jul 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cecheta commented Jun 14, 2022 •

edited

Loading