feat(lambda): event-source maxBatchingWindow property #4260

nmussy · 2019-09-26T15:01:53Z

Implements the new maxBatchingWindow property

added to Kinesis and Dynamo event sources
throw if the property is outside the 300s max bound
remove redundant code in KinesisEventSource and DynamoEventSource into common StreamEventSource

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

mergify · 2019-09-26T15:02:53Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-09-26T15:12:31Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

nmussy · 2019-09-26T17:00:20Z

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

+   * The maximum amount of time to gather records before invoking the function.
+   * Maximum of Duration.minutes(5)
+   *
+   * @default Duration.seconds(0)


I couldn't find documentation on the default, but I'm assuming that since this is a new feature, the previous behavior is the default.

aws-cdk-automation · 2019-09-26T17:11:09Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-09-26T17:55:18Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

nija-at · 2019-09-27T10:42:05Z

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

+   *
+   * @default Duration.seconds(0)
+   */
+  readonly maximumBatchingWindow?: Duration;


Use terser form max instead of maximum everywhere.

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

Pull request has been modified.

aws-cdk-automation · 2019-09-27T12:24:32Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-09-27T12:36:46Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-09-27T12:38:53Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-09-27T13:20:48Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

nija-at · 2019-09-27T15:10:56Z

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

@@ -4,3 +4,4 @@ export * from './sqs';
 export * from './s3';
 export * from './sns';
 export * from './api';
+export * from './stream';


This is an internal refactoring to reduce code duplication. I don't see value in exporting these to consumers (i.e., public API). In fact, it might limit our flexibility to change this in the future.

nija-at · 2019-09-27T15:13:09Z

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

+import lambda = require('@aws-cdk/aws-lambda');
+import {Duration} from "@aws-cdk/core";
+
+export interface StreamingEventSourceProps {


Keep the name consistent - use StreamEventSourceProps.

nija-at · 2019-09-27T15:21:45Z

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

+export abstract class StreamEventSource implements lambda.IEventSource {
+  protected constructor(protected readonly props: StreamingEventSourceProps, protected readonly maxBatchSize: number) {
+    if (this.props.batchSize !== undefined && (this.props.batchSize < 1 || this.props.batchSize > maxBatchSize)) {
+      throw new Error(`Maximum batch size must be between 1 and ${maxBatchSize} inclusive (given ${this.props.batchSize})`);


This feels somewhat over-delegated. Instead of passing maxBatchSize down here, throw this error in each of the subclasses.

nija-at · 2019-09-27T15:25:03Z

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

+    throw new Error('Cannot bind StreamEventSource, use DynamoEventSource or KinesisEventSource');
+  }
+
+  protected getEventSourceMappingOptions(eventSourceArn: string): lambda.EventSourceMappingOptions {


Same here about over-delegation. Don't pass the eventSourceArn here. Instead insert it into the struct, returned by this method, at the calling point in the subclasses.

This would break the requirement of the EventSourceMappingOptions return type, which I'd like to keep. I could change it to Omit<EventSourceMappingOptions, 'eventSourceArn'>, I'll see if JSII is alright with it.

I see, good point.

How about renaming the method signature to enrichMappingOptions(options: lambda.EventSourceMappingOptions) which would add the stream-related properties and return it back?

nija-at · 2019-09-27T15:28:45Z

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

@@ -0,0 +1,52 @@
+import lambda = require('@aws-cdk/aws-lambda');
+import {Duration} from "@aws-cdk/core";


Switch to single quotes.

nija-at · 2019-09-27T15:28:58Z

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

@@ -1,44 +1,26 @@
 import dynamodb = require('@aws-cdk/aws-dynamodb');
 import lambda = require('@aws-cdk/aws-lambda');
+import {StreamEventSource, StreamingEventSourceProps} from "./stream";


Switch to use single quotes

Pull request has been modified.

nmussy · 2019-09-27T17:19:09Z

The PR should be good to go. I was going to create a test suite for stream.ts, but it's an abstract class with a protected constructor.
It's all well covered anyway, except for the unreachable error:

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

Line 43 in d276529

    
           throw new Error('Cannot bind StreamEventSource, use DynamoEventSource or KinesisEventSource');

EDIT: Never mind, I just remembered that TypeScript had abstract methods too. All good!

aws-cdk-automation · 2019-09-27T17:28:03Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-09-27T17:37:06Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-09-28T12:22:53Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2019-09-30T09:58:03Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2019-09-30T10:30:59Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

feat(lambda): event-source maximumBatchingWindow property

f826cae

nmussy requested review from eladb and nija-at as code owners September 26, 2019 15:01

nmussy added 4 commits September 26, 2019 18:54

fix: remove duplicate import

10e1b6e

Merge remote-tracking branch 'upstream/master' into 4257

7551d36

revert: sqs doesn't support maximumBatchingWindow

a0d2f06

chore: set @default

236f0f9

nmussy commented Sep 26, 2019

View reviewed changes

fix: missing import

dfc8c70

SomayaB assigned nija-at Sep 26, 2019

nija-at previously requested changes Sep 27, 2019

View reviewed changes

chore: refactor maximum -> max

5b10aaa

nmussy changed the title ~~feat(lambda): event-source maximumBatchingWindow property~~ feat(lambda): event-source maxBatchingWindow property Sep 27, 2019

chore: refactor kinesis and dynamo in common stream class

47af864

chore: remove space

fffbbc2

chore: fix method order

04045ab

nija-at previously requested changes Sep 27, 2019

View reviewed changes

nija-at reviewed Sep 27, 2019

View reviewed changes

nmussy added 2 commits September 27, 2019 19:01

chore: refactor StreamingEventSourceProps -> StreamEventSourceProps

99aa476

chore: quote fix

9e28c1a

nmussy added 4 commits September 27, 2019 19:04

fix: switch stream.ts to @internal

276c96d

chore: move batchSize check back to children classes

bc92514

chore: more quote fixes

6688d95

chore: refactor enrichMappingOptions

d276529

fix: abstract bind method

178a0c0

chore: remove trailing whitespace

621532e

nija-at approved these changes Sep 30, 2019

View reviewed changes

Merge branch 'master' into 4257

721e03b

mergify bot merged commit 4040032 into aws:master Sep 30, 2019

nmussy mentioned this pull request Sep 30, 2019

chore(lambda): fix StreamEventSourceProps docs #4299

Merged

ashwgupt mentioned this pull request Oct 3, 2019

Stream has no exports #4352

Closed

jogold mentioned this pull request Oct 3, 2019

Missing exports in @aws-cdk/aws-lambda-event-sources v1.11.0 #4358

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lambda): event-source maxBatchingWindow property #4260

feat(lambda): event-source maxBatchingWindow property #4260

nmussy commented Sep 26, 2019 •

edited

Loading

mergify bot commented Sep 26, 2019

aws-cdk-automation commented Sep 26, 2019

nmussy Sep 26, 2019

aws-cdk-automation commented Sep 26, 2019

aws-cdk-automation commented Sep 26, 2019

nija-at Sep 27, 2019

aws-cdk-automation commented Sep 27, 2019

aws-cdk-automation commented Sep 27, 2019

aws-cdk-automation commented Sep 27, 2019

aws-cdk-automation commented Sep 27, 2019

nija-at Sep 27, 2019 •

edited

Loading

nija-at Sep 27, 2019 •

edited

Loading

nija-at Sep 27, 2019

nija-at Sep 27, 2019

nmussy Sep 27, 2019

nija-at Sep 27, 2019 •

edited

Loading

nija-at Sep 27, 2019

nija-at Sep 27, 2019

nmussy commented Sep 27, 2019 •

edited

Loading

aws-cdk-automation commented Sep 27, 2019

aws-cdk-automation commented Sep 27, 2019

aws-cdk-automation commented Sep 28, 2019

mergify bot commented Sep 30, 2019

aws-cdk-automation commented Sep 30, 2019

		@@ -0,0 +1,52 @@
		import lambda = require('@aws-cdk/aws-lambda');
		import {Duration} from "@aws-cdk/core";

feat(lambda): event-source maxBatchingWindow property #4260

feat(lambda): event-source maxBatchingWindow property #4260

Conversation

nmussy commented Sep 26, 2019 • edited Loading

mergify bot commented Sep 26, 2019

aws-cdk-automation commented Sep 26, 2019

AWS CodeBuild CI Report

nmussy Sep 26, 2019

Choose a reason for hiding this comment

aws-cdk-automation commented Sep 26, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Sep 26, 2019

AWS CodeBuild CI Report

nija-at Sep 27, 2019

Choose a reason for hiding this comment

aws-cdk-automation commented Sep 27, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Sep 27, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Sep 27, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Sep 27, 2019

AWS CodeBuild CI Report

nija-at Sep 27, 2019 • edited Loading

Choose a reason for hiding this comment

nija-at Sep 27, 2019 • edited Loading

Choose a reason for hiding this comment

nija-at Sep 27, 2019

Choose a reason for hiding this comment

nija-at Sep 27, 2019

Choose a reason for hiding this comment

nmussy Sep 27, 2019

Choose a reason for hiding this comment

nija-at Sep 27, 2019 • edited Loading

Choose a reason for hiding this comment

nija-at Sep 27, 2019

Choose a reason for hiding this comment

nija-at Sep 27, 2019

Choose a reason for hiding this comment

nmussy commented Sep 27, 2019 • edited Loading

aws-cdk-automation commented Sep 27, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Sep 27, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Sep 28, 2019

AWS CodeBuild CI Report

mergify bot commented Sep 30, 2019

aws-cdk-automation commented Sep 30, 2019

AWS CodeBuild CI Report

nmussy commented Sep 26, 2019 •

edited

Loading

nija-at Sep 27, 2019 •

edited

Loading

nija-at Sep 27, 2019 •

edited

Loading

nija-at Sep 27, 2019 •

edited

Loading

nmussy commented Sep 27, 2019 •

edited

Loading