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(AWS Stream): Add support for custom checkpoint #9056

Merged
merged 10 commits into from Mar 5, 2021

Conversation

imewish
Copy link
Contributor

@imewish imewish commented Mar 3, 2021

Closes: #9030

@codecov
Copy link

codecov bot commented Mar 3, 2021

Codecov Report

Merging #9056 (b81fd05) into master (8592bdb) will increase coverage by 0.12%.
The diff coverage is 95.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9056      +/-   ##
==========================================
+ Coverage   87.04%   87.17%   +0.12%     
==========================================
  Files         287      288       +1     
  Lines       10986    11068      +82     
==========================================
+ Hits         9563     9648      +85     
+ Misses       1423     1420       -3     
Impacted Files Coverage Δ
lib/configuration/variables/parse.js 100.00% <ø> (ø)
lib/utils/open.js 13.58% <66.66%> (ø)
lib/plugins/invoke.js 90.90% <75.00%> (-3.54%) ⬇️
lib/utils/createFromTemplate.js 83.33% <77.27%> (-11.67%) ⬇️
lib/Serverless.js 92.46% <77.77%> (-1.06%) ⬇️
lib/plugins/config.js 81.33% <80.00%> (+0.77%) ⬆️
lib/plugins/create/create.js 90.69% <86.36%> (+0.69%) ⬆️
scripts/serverless.js 85.88% <89.65%> (+11.76%) ⬆️
lib/plugins/package/lib/zipService.js 96.29% <93.65%> (-0.93%) ⬇️
lib/utils/yamlAstParser.js 97.32% <94.82%> (-0.87%) ⬇️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8592bdb...2476f69. Read the comment docs.

Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

Thank you @imewish - awesome job 🙌 I have a few small comments, please let me know what do you think

@@ -572,6 +578,40 @@ describe('AwsCompileStreamEvents', () => {
awsCompileStreamEvents.serverless.service.functions.first.events[4].stream.destinations
.onFailure
);

// event 6
Copy link
Contributor

Choose a reason for hiding this comment

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

All new functionality should be tested using runServerless approach - we're currently in the process of migrating away from these old tests as they're fragile and makes refactorings harder to do. Could you please instead of expanding these, introduce a new test in AwsCompileStreamEvents #2 describe in this module?

For details about approach to testing, please check out: https://github.com/serverless/serverless/blob/master/test/README.md

events:
- stream:
arn: arn:aws:dynamodb:region:XXXXXX:table/foo/stream/1970-01-01T00:00:00.000
functionResponseTypes:
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that underneath it's FunctionResponseTypes, but I think for the Framework for now we should go with simple functionResponseType: ReportBatchItemFailures setting. I know that AWS might expand that in the future, but I think it's very likely that always at most one functionResponseType will be accepted - currently the FunctionResponseTypes is limited to always have at most 1 item.

If AWS ever introduces the option to set multiple types, we will migrate to functionResponseTypes and in the meantime, let's use a simpler approach - what do you think?

Btw - we already followed similar approaches for EFS integration for example.

@@ -32,6 +32,10 @@ class AwsCompileStreamEvents {
maximumRecordAgeInSeconds: {
anyOf: [{ const: -1 }, { type: 'integer', minimum: 60, maximum: 604800 }],
},
functionResponseTypes: {
Copy link
Contributor

Choose a reason for hiding this comment

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

@imewish
Copy link
Contributor Author

imewish commented Mar 3, 2021

Thanks for the feedback @pgrzesik. Let me work on it and will update them soon

@imewish
Copy link
Contributor Author

imewish commented Mar 3, 2021

@pgrzesik pushed some changes as per your comments. Let me know if I'm missing anything.

Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

thanks @imewish 🙇 I have a few minor comments, please let me know what do you think. Additionally, it would be good to extend the serverless.yml.md file with that property: https://github.com/serverless/serverless/blob/master/docs/providers/aws/guide/serverless.yml.md

@@ -32,6 +32,10 @@ class AwsCompileStreamEvents {
maximumRecordAgeInSeconds: {
anyOf: [{ const: -1 }, { type: 'integer', minimum: 60, maximum: 604800 }],
},
functionResponseType: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's expect a string here instead of an array

Copy link
Contributor Author

@imewish imewish Mar 4, 2021

Choose a reason for hiding this comment

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

noted, thought of it will be useful in future if aws add new item :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It will definitely be useful, but I think we can adjust it at that point - I've made a small mistake with my suggestion to change the type to string - following other similar examples, I think we should go with such definition:

functionResponseType: { enum: ['ReportBatchItemFailures'] }

@@ -233,6 +237,10 @@ class AwsCompileStreamEvents {
event.stream.bisectBatchOnFunctionError;
}

if (event.stream.functionResponseType != null) {
streamResource.Properties.FunctionResponseTypes = event.stream.functionResponseType;
Copy link
Contributor

Choose a reason for hiding this comment

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

After reconfiguring schema to expect a string, we will have to wrap it to be an array here I believe

@@ -375,6 +375,7 @@ describe('AwsCompileStreamEvents', () => {
arn: 'arn:aws:dynamodb:region:account:table/bar/stream/2',
batchWindow: 15,
maximumRetryAttempts: 4,
functionResponseType: ['ReportBatchItemFailures'],
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in my previous message, let's not extend these old tests, but instead extend or introduce a new one based on runServerless utility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that. Need to look at the docs and see how it works.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries - let me know if you have any questions around that 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pgrzesik Do we have a new folder to place the new tests? or is it under the same folder?

Copy link
Contributor

Choose a reason for hiding this comment

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

We already have a few of those in a separate describe in this file - please see:

describe('AwsCompileStreamEvents #2', () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks :)

@imewish
Copy link
Contributor Author

imewish commented Mar 4, 2021

Hi @pgrzesik I'm again :), Pushed the changes as per your feedback. https://github.com/serverless/serverless/pull/9056/files. Awaiting feedback.

Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

Well done 🙌 Just two very minor comments and we're ready to merge 👍

@@ -32,6 +32,10 @@ class AwsCompileStreamEvents {
maximumRecordAgeInSeconds: {
anyOf: [{ const: -1 }, { type: 'integer', minimum: 60, maximum: 604800 }],
},
functionResponseType: {
Copy link
Contributor

Choose a reason for hiding this comment

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

It will definitely be useful, but I think we can adjust it at that point - I've made a small mistake with my suggestion to change the type to string - following other similar examples, I think we should go with such definition:

functionResponseType: { enum: ['ReportBatchItemFailures'] }

@@ -233,6 +237,12 @@ class AwsCompileStreamEvents {
event.stream.bisectBatchOnFunctionError;
}

if (event.stream.functionResponseType != null) {
streamResource.Properties.FunctionResponseTypes = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's maybe go with streamResource.Properties.FunctionResponseType = [event.stream.functionResponseType]; to be more succinct here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing. Will be pushing the changes soon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pgrzesik Done :)

Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

Looks awesome, thank you @imewish 🙇

@pgrzesik pgrzesik changed the title DynamoDb/Kinesis Streams Custom Checkpoint feat(AWS Stream): Add support for custom checkpoint Mar 5, 2021
@pgrzesik pgrzesik merged commit b2188a2 into serverless:master Mar 5, 2021
lewgordon pushed a commit to lewgordon/serverless that referenced this pull request Mar 5, 2021
@imewish
Copy link
Contributor Author

imewish commented Mar 6, 2021

Thanks a lot @pgrzesik 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable DynamoDB Streams Custom Checkpointing
2 participants