-
Notifications
You must be signed in to change notification settings - Fork 3.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
fix(custom-resource-handlers): don't recursively process s3 bucket objects #30209
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.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
Exemption Request As I mention in the PR description, this PR makes no changes that can be observed by callers. It's purely an optimization of the implementation. The code being run for each page of bucket objects is identical to what's in If you think more tests are necessary, I'd be happy to take a look, but I'd need your guidance on what is needed. |
Okay, I see there are a bunch of snapshots to update. I had only run unit tests for this package locally. I will work on that. |
Thanks for drafting the fix for this issue. As you described, you're suspecting that recursion is causing this behaviour. I would recommend you to test out your changes locally to verify this actually solves this issue. To test locally, you can clone |
Thanks for the pointer to do that. The other problem is that I’d have to repeatedly generate an s3 bucket with a few hundred thousand objects to test. I suppose that probably doesn’t cost that much money. 🌞 |
In the time it took me to do all that, I am still updating snapshots 😬. I will push them some time. |
134821c
to
30ae85b
Compare
@GavinZZ I might need your help with the one snapshot test that continues to fail:
I don't know anything about ElasticBeanstalk but the error seems unrelated to my change:
|
Someone pointed out on slack that elastic beanstalk has very narrow version support windows. And looking at blame on this test, it seems that whoever happens to invalidate the snapshot is just stuck updating the version number. This meets my criteria for things that should not be snapshot tested 🌞 but to move things along I will just update it. |
30ae85b
to
e869df9
Compare
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
e869df9
to
66fc5c4
Compare
Hi @GavinZZ, could you give this a review, or tell me how to bother another maintainer for one? 🌞 Thanks. |
Hi @GavinZZ, do you know how I can get a code review from a maintainer for this? It has unfortunately started to rot (see the merge conflicts) due to how long it's been waiting. |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
66fc5c4
to
241b46b
Compare
😕 I get the same diff locally, rerun the snapshot test, and it produces the exact same things as are currently snapshotted... it loops forever.
|
…jects I recently had the mispleasure of trying to empty a bucket with ~600000 objects using CDK's `autoDeleteObjects` feature. What I observed was that each lambda invocation would get through a few tens of thousands of objects in relatively good time (a few minutes), then the lambda would grind to a halt doing very little until it reached its 15 minute timeout. This process then repeats with subsequent invocations of the lambda. I suspect but have not proven that the low memory allocated to the lambda (the default 128mb) plus this recursion is to blame. There is no need to recurse, and doing so will put pressure on the stack, the heap, and (because this is an async function) the event loop. I see nothing else that could result in such an outcome here.
241b46b
to
96c5017
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue # (if applicable)
N/A
Reason for this change
I recently had the mispleasure of trying to empty a bucket with ~600000 objects using CDK's
autoDeleteObjects
feature. What I observed was that each lambda invocation would get through a few tens of thousands of objects in relatively good time (a few minutes), then the lambda would grind to a halt doing very little until it reached its 15 minute timeout. This process then repeats with subsequent invocations of the lambda. I had to empty the bucket in the web console to make real progress toward deleting the bucket.I suspect but have not proven that the low memory allocated to the lambda (the default 128mb) plus this recursion is to blame. There is no need to recurse, and doing so will put pressure on the stack, the heap, and (because this is an async function) the event loop. I see nothing else that could result in such an outcome here.
Description of changes
Switch the recursion to iteration.
Description of how you validated changes
I have not added any tests for this change as it is not externally visible to callers at all. Existing test coverage should suffice.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license