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): handle lambda timeout scenarios for INPROGRESS records #1387

Merged

Conversation

rubenfonseca
Copy link
Contributor

@rubenfonseca rubenfonseca commented Jul 27, 2022

Issue number: #1038

Summary

Expire in-progress invocations caused by Lambda time outs.

Changes

Please provide a summary of what's being changed

During an invocation, we try to figure out how much time the Lambda invocation has before it expires, and save it on the idempotency record together will all the other fields.

When a new invocation with the same key arrives, we check against this timestamp to see if there's any INPROGRESS invocation past the timeout timestamp. If there is, we allow the invocation to be retried.

We also moved the squence diagrams of the idempotency module in documentation to mermaid.js.

Caveats

  • Only works out of the box for the @idempotent decorator, because right now it's the only way to access the Lambda context to get the remaining time left. I've added a register_lambda_context to the IdempotencyConfig class as a workaround

carbon (2)

User experience

Please share what the user experience looks like before and after this change

Before: if a Lambda invocation times out, the invocation will get stuck until the idempotency record expires (default = 1 hour).

After: if a Lambda invocation times out, the invocation will be tried again on the next retry.

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

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.


View rendered docs/utilities/idempotency.md

@boring-cyborg boring-cyborg bot added documentation Improvements or additions to documentation tests utilities labels Jul 27, 2022
@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 27, 2022
@github-actions github-actions bot added feature New feature or functionality area/idempotency labels Jul 27, 2022
@rubenfonseca rubenfonseca force-pushed the feat/expire-inprogress branch from de71882 to a4f8ce7 Compare July 27, 2022 08:00
@rubenfonseca rubenfonseca marked this pull request as ready for review July 27, 2022 08:21
@rubenfonseca rubenfonseca requested a review from a team as a code owner July 27, 2022 08:21
@rubenfonseca rubenfonseca requested review from am29d and heitorlessa and removed request for a team and am29d July 27, 2022 08:21
@rubenfonseca rubenfonseca marked this pull request as draft July 27, 2022 08:50
@rubenfonseca rubenfonseca removed the request for review from heitorlessa July 27, 2022 08:50
@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2022

Codecov Report

Merging #1387 (66b62a6) into develop (7414df7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #1387   +/-   ##
========================================
  Coverage    99.88%   99.89%           
========================================
  Files          119      119           
  Lines         5429     5456   +27     
  Branches       620      624    +4     
========================================
+ Hits          5423     5450   +27     
  Misses           2        2           
  Partials         4        4           
Impacted Files Coverage Δ
...ws_lambda_powertools/utilities/idempotency/base.py 100.00% <100.00%> (ø)
..._lambda_powertools/utilities/idempotency/config.py 100.00% <100.00%> (ø)
...da_powertools/utilities/idempotency/idempotency.py 100.00% <100.00%> (ø)
...wertools/utilities/idempotency/persistence/base.py 99.39% <100.00%> (+0.02%) ⬆️
...ools/utilities/idempotency/persistence/dynamodb.py 98.80% <100.00%> (+0.10%) ⬆️
aws_lambda_powertools/tracing/base.py 100.00% <0.00%> (ø)
aws_lambda_powertools/event_handler/appsync.py 100.00% <0.00%> (ø)
aws_lambda_powertools/event_handler/__init__.py 100.00% <0.00%> (ø)

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 c463232...66b62a6. Read the comment docs.

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 27, 2022
@heitorlessa heitorlessa removed their assignment Jul 28, 2022
Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

early comments before I resume reading it after lunch. At a quick glance, the pieces that would benefit the most now are:

  • 1/ Comments on the formula you're using since I saw Lambda context giving us milliseconds, then epoch (sensible for comparison), then type coercion to string (?) - sounds like we could reuse the milliseconds instead of wall clock time. Also, datetime.datetime.now() uses timezone offset too, instead of a safe non-backward timestamp like time.monotonic
  • 2/ I think the Update Expression would benefit to having the new one as a separate variable, then use f-string to combine them and make it more readable (which conditional is for which use case)

aws_lambda_powertools/utilities/idempotency/config.py Outdated Show resolved Hide resolved
tests/functional/idempotency/conftest.py Show resolved Hide resolved
@rubenfonseca rubenfonseca requested a review from heitorlessa July 29, 2022 11:56
@rubenfonseca rubenfonseca requested a review from heitorlessa July 29, 2022 13:19
@heitorlessa
Copy link
Contributor

I've pushed an updated doc with the following changes:

  • Reordered the wording so we give the positive news first (what exactly we do we do?)
  • Give comfort to those using @idempotent (When do they need to care?)
  • Used banners to emphasize when they need to take action (what should I do?)
  • Moved sequence diagram for latter so they're informed first, then they can dig deeper into the mechanics

image

…to feat/expire-inprogress

* rubenfonseca/feat/expire-inprogress:
  chore(idempotency): no need to update expire on update
  chore(documentation): addressed comments
  chore(idempotency): simplified strings
  chore(idempotency): address comments
  chore(docs): add documentation to method
  chore(idempotency): added tests for handle_for_status
@heitorlessa
Copy link
Contributor

Shortening the wording on sequence diagram to fit in the final SVG

New (pushing shortly)

image

@heitorlessa heitorlessa changed the title feat(idempotency): expire time outed in-progress invocations feat(idempotency): handle lambda timeout scenarios for INPROGRESS records Jul 29, 2022
@heitorlessa heitorlessa merged commit 160feae into aws-powertools:develop Jul 29, 2022
@rubenfonseca rubenfonseca deleted the feat/expire-inprogress branch July 29, 2022 13:56
heitorlessa added a commit that referenced this pull request Jul 29, 2022
…tools-python into develop

* 'develop' of https://github.com/awslabs/aws-lambda-powertools-python:
  feat(idempotency): handle lambda timeout scenarios for INPROGRESS records (#1387)
  chore(deps): bump jsii from 1.57.0 to 1.63.1 (#1390)
  chore(deps): bump constructs from 10.1.1 to 10.1.59 (#1396)
  chore(deps-dev): bump flake8-isort from 4.1.1 to 4.1.2.post0 (#1384)
  docs(examples): enforce and fix all mypy errors (#1393)
  chore(ci): drop 3.6 from workflows (#1395)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation feature New feature or functionality size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tests utilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants