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: update Lambda's SQS event batch size and add support to batching window #8555

Merged
merged 8 commits into from
Nov 30, 2020

Conversation

xiqi
Copy link
Contributor

@xiqi xiqi commented Nov 27, 2020

Update SQS event batch size and add support to batching window according to: https://docs.aws.amazon.com/lambda/latest/dg/with-sqs.html

@xiqi xiqi changed the title feat: SQS event batching window and batch size feat: update Lambda's SQS event batch size and add support to batching window Nov 27, 2020
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

@xiqi Thank you, that's nice improvement! Please see my remarks

lib/plugins/aws/package/compile/events/sqs/index.js Outdated Show resolved Hide resolved
docs/providers/aws/events/sqs.md Outdated Show resolved Hide resolved
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

@xiqi great thanks for update, and sorry for another turnaround (I didn't look close enough at first time).

To be consistent we need to tackle two more things.

lib/plugins/aws/package/compile/events/sqs/index.js Outdated Show resolved Hide resolved
lib/plugins/aws/package/compile/events/sqs/index.js Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Nov 28, 2020

Codecov Report

Merging #8555 (d5f7902) into master (d1a22c8) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8555   +/-   ##
=======================================
  Coverage   87.27%   87.27%           
=======================================
  Files         256      256           
  Lines        9516     9516           
=======================================
  Hits         8305     8305           
  Misses       1211     1211           
Impacted Files Coverage Δ
...ib/plugins/aws/package/compile/events/sqs/index.js 100.00% <100.00%> (ø)
lib/plugins/aws/provider/awsProvider.js 93.06% <0.00%> (ø)

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 d1a22c8...d5f7902. Read the comment docs.

@xiqi xiqi requested a review from medikoo November 28, 2020 06:42
Copy link
Contributor

@medikoo medikoo 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 @xiqi looks really good. I've proposed just some cleanup, so we land with more straightforward construct.

Let me know what you think

lib/plugins/aws/package/compile/events/sqs/index.js Outdated Show resolved Hide resolved
docs/providers/aws/events/sqs.md Outdated Show resolved Hide resolved
@xiqi xiqi requested a review from medikoo November 29, 2020 13:30
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

@xiqi it looks great! I've just spotted one omission in schema configuration, and can we have new property be backed by unit test? For that ideally extend already existing runServerless run here:

const { awsNaming, cfTemplate } = await runServerless({
fixture: 'function',
configExt: {
functions: {
foo: {
provisionedConcurrency: 1,
events: [{ sqs: 'arn:aws:sqs:region:account:MyQueue' }],
},
},
},
cliArgs: ['package'],
});

  1. Configure new function (in configExt property), which would have sns event and maximumBatchingWindow property
  2. Then in dedicated it confirm that generated CF resource has expected property and value

For more info on tests, check: https://github.com/serverless/serverless/tree/c9da466875da95584c754b991ec1646998f10018/test#unit-tests

lib/plugins/aws/package/compile/events/sqs/index.js Outdated Show resolved Hide resolved
@xiqi xiqi requested a review from medikoo November 30, 2020 15:07
Copy link
Contributor

@medikoo medikoo 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 @xiqi looks great! I've just proposed to not use JSON.stringify it doesn't seem justified to confirm on JSON stringified values

lib/plugins/aws/package/compile/events/sqs/index.test.js Outdated Show resolved Hide resolved
@xiqi xiqi requested a review from medikoo November 30, 2020 15:34
Copy link
Contributor

@medikoo medikoo 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 @xiqi !

@medikoo medikoo merged commit ffde506 into serverless:master Nov 30, 2020
ojongerius added a commit to ojongerius/serverless that referenced this pull request Dec 15, 2020
ojongerius added a commit to ojongerius/serverless that referenced this pull request Dec 15, 2020
ojongerius added a commit to ojongerius/serverless that referenced this pull request Dec 15, 2020
ojongerius added a commit to ojongerius/serverless that referenced this pull request Dec 15, 2020
ojongerius added a commit to ojongerius/serverless that referenced this pull request Dec 15, 2020
ojongerius added a commit to ojongerius/serverless that referenced this pull request Dec 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants