-
Notifications
You must be signed in to change notification settings - Fork 0
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: lambda worker - support maxConcurrency (SqsEventSource) #86
feat: lambda worker - support maxConcurrency (SqsEventSource) #86
Conversation
2125fac
to
b535f2c
Compare
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.
I think that's looking ok for exposing the new optional property, I think all this now needs is the tests updating, to verify that if you set the property the it really is exposed:
@@ -141,6 +141,7 @@ export class LambdaWorker extends Construct { | |||
new SqsEventSource(lambdaQueue, { | |||
enabled: props.lambdaProps.enableQueue ?? true, | |||
batchSize: 1, | |||
maxConcurrency: props.lambdaProps.queueMaxConcurrency, |
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.
what is the default value maxConcurrency
gets set to if you don't specify a value?
does it need a default value i.e. maxConcurrency: props.lambdaProps.queueMaxConcurrency ?? <some default>
, or do we only set the property if a value for queueMaxConcurrency
is set?
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.
By default, maxConcurrency has no specific limit - uses up to maximum concurrency allowed by AWS.
I would add default value only if we internally know that there is some value that suits us for our use cases. Otherwise, I think that just forwarding a parameter and leaving default behavior is the best option.
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.
Lets not make this an optional property, lets make it a mandatory property. This is a breaking change, but will force us to enumerate the max concurrency for every use case, which is actually the right thing to do.
b535f2c
to
457459e
Compare
457459e
to
cc2502d
Compare
🎉 This PR is included in version 3.2.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
resolves: https://techfromsage.atlassian.net/browse/PLT-83
This PR adds support for maxConcurrency property in LambdaWorker (SqsEventSource)