Skip to content
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 helper utility #245

Merged
merged 47 commits into from Feb 19, 2021
Merged

feat: Idempotency helper utility #245

merged 47 commits into from Feb 19, 2021

Conversation

to-mc
Copy link
Contributor

@to-mc to-mc commented Dec 16, 2020

Issue #, if available: #218

Description of changes:

Implementing RFC #218. I'll be working together with @igorlg on this.

Todos

  • Implement caching
  • Update caching implementation with LRU / max size
  • Review/replace error handling implementation (especially the pickle bit, which currently causes security baseline to fail the build)
  • Add unit tests and complete integration tests
  • Review/remove table creation logic/implementation
  • Add logging
  • Add docs
  • Add payload validation
  • Improve exception handling for unrecoverable errors from persistence layer

Checklist

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@to-mc to-mc marked this pull request as draft December 16, 2020 13:20
@to-mc to-mc changed the title [DRAFT] feat: initial commit for idempotency utility feat: Idempotency helper utility Dec 16, 2020
@to-mc to-mc added area/utilities feature New feature or functionality labels Dec 16, 2020
@codecov-io
Copy link

codecov-io commented Jan 15, 2021

Codecov Report

Merging #245 (d17275d) into develop (fc08062) will decrease coverage by 0.51%.
The diff coverage is 93.90%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #245      +/-   ##
===========================================
- Coverage    99.75%   99.23%   -0.52%     
===========================================
  Files           79       85       +6     
  Lines         2859     3138     +279     
  Branches       119      151      +32     
===========================================
+ Hits          2852     3114     +262     
- Misses           5       14       +9     
- Partials         2       10       +8     
Impacted Files Coverage Δ
aws_lambda_powertools/shared/cache_dict.py 71.42% <71.42%> (ø)
...da_powertools/utilities/idempotency/idempotency.py 92.53% <92.53%> (ø)
...wertools/utilities/idempotency/persistence/base.py 95.16% <95.16%> (ø)
...ambda_powertools/utilities/idempotency/__init__.py 100.00% <100.00%> (ø)
...bda_powertools/utilities/idempotency/exceptions.py 100.00% <100.00%> (ø)
...ools/utilities/idempotency/persistence/dynamodb.py 100.00% <100.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc08062...d17275d. Read the comment docs.

Copy link
Contributor

@michaelbrewer michaelbrewer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some comments so far.

Copy link
Contributor

@michaelbrewer michaelbrewer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cakepietoast I added some updated comments.

@github-actions github-actions bot added documentation Improvements or additions to documentation and removed area/utilities labels Feb 15, 2021
aws_lambda_powertools/shared/cache_dict.py Show resolved Hide resolved
Comment on lines 49 to 56
**Processes Lambda's event in an idempotent manner**
>>> from aws_lambda_powertools.utilities.idempotency import idempotent, DynamoDBPersistenceLayer
>>>
>>> persistence_store = DynamoDBPersistenceLayer(event_key="body", table_name="idempotency_store")
>>>
>>> @idempotent(persistence_store=persistence_store)
>>> def handler(event, context):
>>> return {"StatusCode": 200}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Should we explain how it'll work under the hood here? This could help clarify how often this will call DynamoDB, what parameters it'll look at to decide if it's an idempotent request or not, etc.

Question: Should we mention how this stores the event into DynamoDB?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we address this it should be in the documentation. Happy to make changes to the docs based on feedback!

aws_lambda_powertools/utilities/idempotency/idempotency.py Outdated Show resolved Hide resolved
aws_lambda_powertools/utilities/idempotency/idempotency.py Outdated Show resolved Hide resolved
Copy link
Contributor

@nmoutschen nmoutschen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for the doc. Just 2 nitpicks, but these can be changed in the future.

docs/utilities/idempotency.md Outdated Show resolved Hide resolved
docs/utilities/idempotency.md Outdated Show resolved Hide resolved
Copy link
Contributor

@michaelbrewer michaelbrewer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small review changes

Tom McCarthy and others added 5 commits February 19, 2021 17:09
Co-authored-by: Michael Brewer <michael.brewer@gyft.com>
Co-authored-by: Michael Brewer <michael.brewer@gyft.com>
Co-authored-by: Michael Brewer <michael.brewer@gyft.com>
Co-authored-by: Michael Brewer <michael.brewer@gyft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants