-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Conversation
There was a problem hiding this 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
There was a problem hiding this 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.
Codecov Report
@@ Coverage Diff @@
## master #8555 +/- ##
=======================================
Coverage 87.27% 87.27%
=======================================
Files 256 256
Lines 9516 9516
=======================================
Hits 8305 8305
Misses 1211 1211
Continue to review full report at Codecov.
|
There was a problem hiding this 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
There was a problem hiding this 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:
serverless/lib/plugins/aws/package/compile/events/sqs/index.test.js
Lines 594 to 605 in c9da466
const { awsNaming, cfTemplate } = await runServerless({ | |
fixture: 'function', | |
configExt: { | |
functions: { | |
foo: { | |
provisionedConcurrency: 1, | |
events: [{ sqs: 'arn:aws:sqs:region:account:MyQueue' }], | |
}, | |
}, | |
}, | |
cliArgs: ['package'], | |
}); |
- Configure new function (in
configExt
property), which would havesns
event andmaximumBatchingWindow
property - 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
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @xiqi !
Update SQS event batch size and add support to batching window according to: https://docs.aws.amazon.com/lambda/latest/dg/with-sqs.html