fix: cold start only on on-demand invocation #2317
Conversation
dreamorosi
left a comment
dreamorosi
left a comment
There was a problem hiding this comment.
Hi @Attyuttam thank you for the PR.
Before we can start with the review, please make sure that the project still compiles after having made changes. Currently the CI is failing across the board.
If you have questions, please let us know.
|
@Attyuttam The CI fails due to broken unit tests. You can run locally My suggestion is to update the failing unit test by setting the environment variable you are checking for. You can use |
|
Still working on this, there are a lot of test failures due to the code change, will update and drop a message here ! |
|
No worries, feel free to reach out if you have any questions. Your work on this is much appreciated 👍 |
|
@Attyuttam For the other tests that are failing, most likely the issue will be resolved if you configure in |
|
Ohh, okay. I was manually setting using the @SetEnvironmentVariable annotation on every test, I was about to ask this here. Thanks for the suggestion. |
|
the powertools-idempotency-dynamodb module was failing in mvn clean test. Although, when the tests were run independently, they passed. I looked at the blog that was put in the comments but that has become invalid, so I updated that to the correct URL as well. I tried incorporating the changes based on the latest blog by nickolas fisher, but it did not work. Can you please help me out here |
|
phipag
left a comment
phipag
left a comment
There was a problem hiding this comment.
Hey @Attyuttam! Apologies for the late reply, I am back from travels now.
I left some comments below. I think the dynamodb test as well as some of the logging assertions are failing due to your operating system (I assume it is Windows?). Currently, the project is optimized to run on unix-like systems. Do you have the possibility to run this on WSL or a unix-like machine? If not, I am happy to verify your changes on my computer and send potential last changes myself manually. Let me know how you want to proceed.
There was a problem hiding this comment.
Please revert all changes in this file.
There was a problem hiding this comment.
Please revert all changes in this file.
There was a problem hiding this comment.
Please revert all changes in this file. (the addition of \r)
There was a problem hiding this comment.
Please revert all changes in this file. (the addition of \r)
There was a problem hiding this comment.
Please revert all changes in this file.
There was a problem hiding this comment.
Please revert all changes in this file. This is not needed in the parent pom. Please make sure to add it in the corresponding module poms such as all the logging modules.
|
Hey @phipag ! hope you had a good time !! It would be great if you could run it in your system, I unfortunately have a windows machine |
|
Thanks @Attyuttam. I moved over your commits to this PR (because I don't have permission on your fork): #2329. I will make sure your contribution is recognized with your account! Thanks again! Please feel free to comment / approve this PR. |
3 participants
Summary
Changes
Fixes #2113 - cold start only for on-demand invocation type else its not considered
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.