-
Notifications
You must be signed in to change notification settings - Fork 411
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
feat(idempotency): leverage new DynamoDB Failed conditional writes behavior with ReturnValuesOnConditionCheckFailure #3446
feat(idempotency): leverage new DynamoDB Failed conditional writes behavior with ReturnValuesOnConditionCheckFailure #3446
Conversation
… to return a copy of the item on failure and avoid a subsequent get aws-powertools#3327
… to return a copy of the item on failure and avoid a subsequent get aws-powertools#3327
Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need. |
SonarCloud Quality Gate failed.
|
hey @dastra stellar work - setting expectation that we might be able to review and give you feedback next week only; we're catching up to the re:Invent backlog as the team was out. |
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #3446 +/- ##
===========================================
+ Coverage 95.56% 95.57% +0.01%
===========================================
Files 213 214 +1
Lines 9961 9993 +32
Branches 1824 1827 +3
===========================================
+ Hits 9519 9551 +32
Misses 329 329
Partials 113 113 ☔ View full report in Codecov by Sentry. |
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.
Hello @dastra! This is a great job and a great improvement for our customers, after all, we are reducing costs by removing a get_item
call and even reducing network latency, as now the data stored in the DDB is returned in a single call.
I made some comments but this PR is super! I'll make sure we have this merged as soon as possible.
aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/idempotency/persistence/base.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.py
Outdated
Show resolved
Hide resolved
… to return a copy of the item on failure and avoid a subsequent get aws-powertools#3327. Changes after PR comments
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.
Hello @dastra! Thanks for addressing the last few review comments! I just left a comment about a small change.
I'm still working on our documentation because now we have 3 possible workflows for successful executions, and each of them makes a different number of requests. So we must document it properly to make it clear to our customers.
First execution
PutItem
(1 DDB request) -> execute Lambda -> UpdateItem
with Lambda response (1 DDB request) = 2 requests total
Second execution (record exists and boto3 supports ReturnValuesOnConditionCheckFailure
)
PutItem
(1 DDB request) -> The conditional check fails with existing record -> Return Item from exception = 1 request total
Second execution with legacy behavior (record exists and boto3 doesn't support ReturnValuesOnConditionCheckFailure
)
PutItem
(1 request) - The conditional check fails with the existing record -> GetItem
(1 DDB request) = 2 requests total
Items missing before we merge this:
|
…ired_during_request Co-authored-by: Leandro Damascena <[email protected]> Signed-off-by: Dan Straw <[email protected]>
This is ready for review @rubenfonseca! Please review it. |
aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.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.
Sorry, just one more tiny thing
aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.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.
Thank you for this stellar work @dastra!
APPROVED!
Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience! |
Issue number: #3327
Summary
Changes
This feature passes ReturnValuesOnConditionCheckFailure when putting the idempotency record in DynamoDB. If the put fails, then a copy of the item as it was during the failed write attempt is returned in the response. This avoids having to perform a separate get request to retrieve the item.
Detail:
User experience
The user experience is identical before and after the change. However as there is no subsequent get when the conditional check fails, there is no associated RCU consumed.
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change?
RFC issue number:
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.