-
Notifications
You must be signed in to change notification settings - Fork 402
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: correct behaviour to avoid caching "INPROGRESS" records #295
Conversation
@cakepietoast haha. that was quick :) |
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.
LGTM
@cakepietoast @heitorlessa maybe to help with code review we can try to use more GIVEN, WHEN & THENs. At least just to cover the basic behavors we put into the RFCs. Coz then we are less likely to miss things |
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.
LGTM!
aws_lambda_powertools/utilities/idempotency/persistence/base.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/idempotency/persistence/base.py
Outdated
Show resolved
Hide resolved
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.
I have attached my proposal changes
@cakepietoast @heitorlessa I spend a little time on this and attached a diff of changes Changes:base.py
test_idempotency.py
test_json_encoder.py
Attached diff |
Codecov Report
@@ Coverage Diff @@
## develop #295 +/- ##
===========================================
+ Coverage 99.64% 99.71% +0.06%
===========================================
Files 86 86
Lines 3142 3143 +1
Branches 151 149 -2
===========================================
+ Hits 3131 3134 +3
+ Misses 6 5 -1
+ Partials 5 4 -1
Continue to review full report at Codecov.
|
Yep, you're completely right on this. Its been on my todo list with this PR since the beginning but it got lost amongst the rest. I'll get this added as soon as I get chance. |
Issue #, if available:
Description of changes:
This fixes an issue with caching in the unreleased idempotency util. We were mistakenly caching in progress records, meaning execution environments could get "stuck" with in progress records in the cache. Records in this state shouldn't be included in the cache, as we've no way to reflect changes from outside the execution environment in the local caches.
Checklist
Breaking change checklist
RFC issue #:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.