Queue: Add setting for maximumBatchingWindow #161
Conversation
src/constructs/aws/Queue.ts
Outdated
| @@ -130,6 +137,13 @@ export class Queue extends AwsConstruct { | |||
| // The default function timeout is 6 seconds in the Serverless Framework | |||
| const functionTimeout = configuration.worker.timeout ?? 6; | |||
|
|
|||
| const maximumBatchingWindow = | |||
There was a problem hiding this comment.
I would probably extract this in a private method as it's calculated 2 times
src/constructs/aws/Queue.ts
Outdated
| @@ -240,6 +252,7 @@ export class Queue extends AwsConstruct { | |||
| private appendFunctions(): void { | |||
| // The default batch size is 1 | |||
| const batchSize = this.configuration.batchSize ?? 1; | |||
| const maximumBatchingWindow = this.queue.fifo ? 0 : this.configuration.maxBatchingWindow ?? MAX_BATCHING_WINDOW; | |||
There was a problem hiding this comment.
It was previously set to undefined if FIFO is enabled. Why the change to 0?
There was a problem hiding this comment.
I can set it back to undefined no problem.
The reason why I set it to 0 was from the docs it says that 0 is the default: https://docs.aws.amazon.com/lambda/latest/dg/with-sqs.html#services-sqs-params
And TypeScript was complaining in that unary expression :)
| const maximumBatchingWindow = | ||
| configuration.fifo === true ? 0 : configuration.maxBatchingWindow ?? MAX_BATCHING_WINDOW; | ||
|
|
||
| // This should be 6 times the lambda function's timeout + MaximumBatchingWindowInSeconds |
docs/queue.md
Outdated
| maxBatchingWindow: 5 # SQS will wait 5 seconds (so that it can batch any messages together) before delivering to lambda | ||
| ``` | ||
|
|
||
| *Default: 60 seconds* |
There was a problem hiding this comment.
Also I'm wondering if the default for maxBatchWindow should be 0 rather than 60 as it might catch people out?
Actually I agree with you… CloudFormation says the default is 0 for SQS:
I would say we should switch to 0, WDYT @t-richard?
There was a problem hiding this comment.
AFAIK the batching window is mostly for cost/usage optimization. It will wait longer so it can eventually gather more messages for a batch which will avoid too much Lambda invocations.
Also, setting this to something else than 0 allows for batches of more than 10 items which we don't currently allow (but maybe we should 🙂).
There's no clear answer here, it depends on the use case people make from the queue. Sometimes performance will be prefered, sometimes usage optimization.
If I had to make a choice, I'd keep it as is. 60 seconds seems like a good default to me because it allows for better optimization without impacting performance too much. It will have no impact for batchSize of 1 and most of the time you are seeking optimization when using batches.
There was a problem hiding this comment.
Thanks, I wasn't sure if there were scenarios I was missing.
Let's use 0 as default then, because:
- it's consistent with the actual default from AWS
- it's not surprising (anything else is surprising because it makes the queue non-real-time, which isn't stated upfront in this construct)
As you mentioned, setting any other value is an optimization that users can make based on the specific usage for their queue, so let's keep it real-time by default.
There was a problem hiding this comment.
I'll update the PR with 0 as the default.
| }); | ||
| }); | ||
|
|
||
| it("sets the SQS visibility timeout to 6 times the function timeout + max batching window in seconds when using custom maxBatchingWindow", async () => { |
…eues are processed in real time
|
Hello, is there any news regarding the release of this modification? |
|
Hello guys! I really need to change the value of maximumBatchingWindow , do you need any help in order to complete this merge? |
|
Please can we merge this and get a release out? Really need this change |
|
Looking great, thank you! |
I added an issue for this here #160