feat(lambda-event-sources): add sqs provisioned poller config#36990
feat(lambda-event-sources): add sqs provisioned poller config#36990
Conversation
github-actions
bot
added
p2
beginning-contributor
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
|
||||||||||||||
|
|
||||||||||||||
aws-cdk-automation
added
the
pr/needs-community-review
badmintoncryer
left a comment
badmintoncryer
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This PR looks almost good. I've add some minor comments.
| const app = new cdk.App({ | ||
| postCliContext: { | ||
| '@aws-cdk/aws-lambda:useCdkManagedLogGroup': false, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Is this feature flag needed?
| const app = new cdk.App({ | |
| postCliContext: { | |
| '@aws-cdk/aws-lambda:useCdkManagedLogGroup': false, | |
| }, | |
| }); | |
| const app = new cdk.App(); |
There was a problem hiding this comment.
Thanks for pointing this out. I followed the same pattern as other integration tests in this directory (e.g., integ.sqs-null-filter.ts), which also sets postCliContext with @aws-cdk/aws-lambda:useCdkManagedLogGroup: false. I'm happy to remove it if you think consistency with those existing tests isn't necessary here — what would you prefer?
There was a problem hiding this comment.
This flag controls whether CDK auto-creates a LogGroup for Lambda. It's unrelated to Provisioned Pollers, so please just use new cdk.App(). The other tests likely just copied this pattern without need.
| testCases: [stack], | ||
| }); | ||
|
|
||
| app.synth(); |
There was a problem hiding this comment.
This line is not needed.
| app.synth(); |
There was a problem hiding this comment.
Same as above — this follows the pattern in integ.sqs-null-filter.ts and other existing integration tests that include app.synth(). I can remove it if you'd like, but wanted to flag the existing convention. Let me know your preference and I'll align accordingly.
There was a problem hiding this comment.
app.synth() used to be required, but it became unnecessary a few years ago. For recent integ tests, please omit it.
| const fn = new TestFunction(this, 'F'); | ||
| const queue = new sqs.Queue(this, 'Q', { |
There was a problem hiding this comment.
nit: I think integration tests may also be referenced as working examples. In that case, having only Q or F written as an ID seems slightly problematic. Could you replace it with an appropriate CamelCase word?
There was a problem hiding this comment.
Good point! I've updated the construct IDs to use descriptive CamelCase names. Please take a look.
| } | ||
| if (this.props.provisionedPollerConfig) { | ||
| const { minimumPollers, maximumPollers } = this.props.provisionedPollerConfig; | ||
| if (minimumPollers !== undefined) { |
There was a problem hiding this comment.
We should consider the case which the value is a token.
| if (minimumPollers !== undefined) { | |
| if (minimumPollers !== undefined && !Token.isUnresolved(minimumPollers)) { |
There was a problem hiding this comment.
Addressed. The validation now skips checks when the value is an unresolved token, using Token.isUnresolved(). This applies to minimumPollers, maximumPollers, and the comparison between them. Please verify the implementation.
| throw new ValidationError('Minimum provisioned pollers for SQS must be between 2 and 200 inclusive', queue); | ||
| } | ||
| } | ||
| if (maximumPollers !== undefined) { |
There was a problem hiding this comment.
same as above. Please consider token case.
| throw new ValidationError('Maximum provisioned pollers for SQS must be between 2 and 2000 inclusive', queue); | ||
| } | ||
| } | ||
| if (minimumPollers !== undefined && maximumPollers !== undefined) { |
There was a problem hiding this comment.
Same as above. Please consider token case.
| throw new ValidationError(`Maximum batch size must be between 1 and 10 inclusive (given ${this.props.batchSize}) when batching window is not specified.`, queue); | ||
| } | ||
| } | ||
| if (this.props.provisionedPollerConfig && this.props.maxConcurrency !== undefined) { |
There was a problem hiding this comment.
The implementation is such that CDK does not throw an error when provisionedPollerConfig is an empty object {}. Is this acceptable? Is an actual deployment possible?
Personally, even if deployment with default values is possible, I feel it's not very intuitive that a configuration using an empty object is allowed.
There was a problem hiding this comment.
Thanks for raising this. According to the AWS Lambda API documentation, both MinimumPollers and MaximumPollers are marked as Required: No. So passing ProvisionedPollerConfig as an empty object should enable provisioned mode with Lambda's default values (minimumPollers: 2, maximumPollers: 200).
If we were to require at least one property, it would diverge from the API's actual behavior — users wouldn't be able to simply "enable provisioned mode with defaults" by passing provisionedPollerConfig: {}.
That said, I understand the concern about intuitiveness. As a middle ground, I've documented this behavior in the JSDoc for SqsProvisionedPollerConfig, noting that an empty object enables provisioned mode with Lambda's default values. Would that work, or would you prefer to enforce at least one property? I'll defer to your judgment here.
There was a problem hiding this comment.
I understand! It's ok to configure default value by passing {}.
| if (this.props.provisionedPollerConfig && this.props.maxConcurrency !== undefined) { | ||
| throw new ValidationError('provisionedPollerConfig and maxConcurrency are mutually exclusive', queue); | ||
| } | ||
| if (this.props.provisionedPollerConfig) { |
There was a problem hiding this comment.
Should we add a integer validation for minimum/maximumPollers?
There was a problem hiding this comment.
Added. The validation now uses Number.isInteger() to ensure minimumPollers and maximumPollers are integers. Please check the implementation.
… in SQS provisioned poller config
…pollers integ test
…QS provisioned poller config
…sionedPollerConfig
|
@badmintoncryer |
|
@kawaaaas Thanks! I want you to update integ test and will approve this PR. |
…nth from integ test
|
@badmintoncryer Thank you for the review! Updated the integ test and snapshot. |
|
Nice work! Would love to see this get merged. I went looking for this today and I guess I will need to use an escape hatch for now |
4 participants
Issue # (if applicable)
None
Reason for this change
Provisioned Mode for SQS Event Source Mappings is not yet supported in the L2 SqsEventSource construct. Users who want to use provisioned pollers must currently resort to L1 (CfnEventSourceMapping) or escape hatches, losing the benefit of type-safe props and built-in validation that L2 constructs provide.
Description of changes
Added
provisionedPollerConfigproperty toSqsEventSourceProps, allowing users to configureminimumPollersandmaximumPollersfor SQS event source mappings.Validation
minimumPollers: must be between 2 and 200 inclusivemaximumPollers: must be between 2 and 2,000 inclusiveminimumPollersmust be ≤maximumPollersprovisionedPollerConfigandmaxConcurrencyare mutually exclusiveThe validation pattern follows the existing approach used for Kafka event sources.
Design decision
Although a
ProvisionedPollerConfiginterface already exists in both theaws-lambdamodule (event-source-mapping.ts) and theaws-lambda-event-sourcesmodule (stream.ts), this PR defines a separateSqsProvisionedPollerConfiginterface withinaws-lambda-event-sources(sqs.ts). This keeps the SQS-specific validation boundaries separate from stream sources, which may have different constraints. This separation of concerns avoids coupling SQS configuration to stream source behavior and makes it easier to evolve each independently.Describe any new or updated permissions being added
None
Description of how you validated changes
Add both unit and integ tests
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license