-
Notifications
You must be signed in to change notification settings - Fork 4.3k
chore(s3-deployment): increase default memory limit from 128MB to 512MB #35501
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
base: main
Are you sure you want to change the base?
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.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Comments on closed issues and PRs are hard for our team to see. |
As discussed #35487 (comment) changing this default value would be a breaking change, I think we should introduce a feature flag for that. |
@@ -362,7 +362,7 @@ new s3deploy.BucketDeployment(this, 'DeployWithSignedPayloads', { | |||
|
|||
## Size Limits | |||
|
|||
The default memory limit for the deployment resource is 128MiB. If you need to | |||
The default memory limit for the deployment resource is 512MiB when the feature flag `@aws-cdk/aws-s3-deployment:defaultMemoryLimit` is enabled, otherwise 128MiB. If you need to |
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.
The default memory limit for the deployment resource is 512MiB when the feature flag `@aws-cdk/aws-s3-deployment:defaultMemoryLimit` is enabled, otherwise 128MiB. If you need to | |
The default memory limit for the deployment resource is 512MiB. If you need to |
Let's make it this way as the FF default value is true. We don't need to mention FF in the README.
@@ -154,7 +156,7 @@ export interface BucketDeploymentProps { | |||
* If you are deploying large files, you will need to increase this number | |||
* accordingly. | |||
* | |||
* @default 128 | |||
* @default 512 when feature flag `@aws-cdk/aws-s3-deployment:defaultMemoryLimit` is enabled, otherwise 128 |
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.
* @default 512 when feature flag `@aws-cdk/aws-s3-deployment:defaultMemoryLimit` is enabled, otherwise 128 | |
* @default 512 |
@@ -78,5 +78,6 @@ | |||
"@aws-cdk/s3-notifications:addS3TrustKeyPolicyForSnsSubscriptions": true, | |||
"@aws-cdk/aws-ec2:requirePrivateSubnetsForEgressOnlyInternetGateway": true, | |||
"@aws-cdk/aws-s3:publicAccessBlockedByDefault": true, | |||
"@aws-cdk/aws-s3-deployment:defaultMemoryLimit": true, |
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.
@aws-cdk/aws-s3-deployment:defaultMemoryLimit
this is not descriptive for me. How about rename to
@aws-cdk/aws-s3-deployment:default512MemoryLimit ?
And update all occurrences?
5dcc65b
to
4a99c01
Compare
- Update BucketDeployment default memory limit from 128MB to 512MB for better S3 sync performance - Add test to verify new default memory behavior - Update documentation to reflect new default - Maintain backward compatibility for explicit memory configurations Fixes aws#35487
- Add @aws-cdk/aws-s3-deployment:defaultMemoryLimit feature flag - When enabled: uses 512MB default memory (performance optimized) - When disabled: uses 128MB default memory (legacy behavior) - Maintains full backward compatibility - Users can still override with explicit memoryLimit values - Addresses PR feedback about breaking changes Closes aws#35487
…test - Add new feature flag to expected defaults in features.test.ts - Feature flag defaults to false (disabled) for backward compatibility - Fixes CI test failure for feature flag defaults validation
- Rename feature flag from 'defaultMemoryLimit' to 'default512MemoryLimit' for clarity - Simplify README documentation to remove feature flag mention since it defaults to true - Simplify JSDoc comment to show '@default 512' without feature flag details - Update all references to use the new feature flag name
4a06ffc
to
9fdf3b5
Compare
…t memory limit - Add explicit MemorySize: 128 property to BucketDeployment Lambda function - This reflects the feature flag implementation that makes memory allocation explicit
- Add @aws-cdk/aws-s3-deployment:configurable-bucket-deployment-memory feature flag - Update BucketDeployment to use feature flag for memory configuration - Maintain backward compatibility with existing behavior - Update integration tests and snapshots
Issue # (if applicable)
Closes #35487.
Reason for this change
The BucketDeployment construct's Lambda custom resource uses a default memory limit of 128MB, which causes extremely poor S3 sync performance (10s of KB/s) and deployment timeouts. Users reported 15-minute timeouts when deploying typical static website assets, making the construct unreliable for production use without manual memory configuration.
Description of changes
Increased the default memory limit for BucketDeployment Lambda custom resource from 128MB to 512MB to provide dramatically improved S3 sync performance out of the box.
BucketDeploymentProps.memoryLimit
default from 128MB to 512MB using nullish coalescing operator@default 512
) and README.md to reflect new defaultDescribe any new or updated permissions being added
N/A - No IAM permissions or resource access changes. This is purely a Lambda memory configuration adjustment.
Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license