-
Notifications
You must be signed in to change notification settings - Fork 36
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 assetlibrary history batching #88
base: main
Are you sure you want to change the base?
Fix assetlibrary history batching #88
Conversation
Any questions or feedback on this PR that I can help to answer? |
This PR has been sitting for quite a while. Any chance this is even on the radar to get merged in? |
I see there's some movement on this repo again! Is there an appetite to merge in this Pull request (or any other community pull requests)? |
One last effort here to get any kind of feedback from anyone on the team, otherwise we will just fork this and manage it on our own. Is this even worth me going through and fixing all this stuff so that we can get this in? |
@aaronatbissell, as with the other PR, we have resumed reviewing PRs and will be happy to review this once you've updated the conflicts. I am sorry for the delay in response, as we were undergoing a team change. |
Hello @aaronatbissell, are you planning to work on this PR? We plan to close this on 9/8/23 for tracking purposes if it's not still active (in that case, please re-open it if/when you have update it). Thank you |
Yes - I do plan on working this. I just don't have the time to do it right now. It will probably be in a month or so |
97a0eac
to
b26183c
Compare
@ts-amz - rebased on main |
@aaronatbissell do you know how we can test this before merging? |
@canavandl This is going back a bit for me and I no longer use cdf but I believe @aaronatbissell and I were able to deploy these changes to a lower environment and make sure that asset library history table was being updated with changes. It might be more difficult to test large batches. Maybe set the |
…y history updates ISSUES CLOSED: aws#87
aee3370
to
0a74dc1
Compare
@ts-amz @canavandl - FYI, just made a couple more updates to this. Unit tests are included with #193. We've pushed this out to our system to ensure it's working. I think this is ready for an in-depth review and merge. |
After running for a few days in our production environment, this asset library history batching has reduced our total lambda duration by about 60% and reduced invocations by 99%. |
Description
As @aaronatbissell mentioned in #87 - Large amounts of assetlibrary changes can cause hitting lambda concurrency limits. This proposed change utilizes a Kinesis Stream to batch updates to batches of 100 or 30 second window (These can be configured differently if needed)
Type of change
Submission Checklist
Additional Notes: