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): adding redis as idempotency backend #2567

Merged
merged 123 commits into from
Jan 10, 2024

Conversation

roger-zhangg
Copy link
Member

@roger-zhangg roger-zhangg commented Jun 23, 2023

Issue number: #1181
previous pr: #1914

Summary

PR adds support for Redis as Idempotecny backend

Changes

Please provide a summary of what's being changed

The following features for Redis are currently supported by this PR

  • Add, update, get and delete idempotency record.
  • Standalone and Cluster architecture for Redis. The third type Sentinel will be added to this soon.
  • The database index, which is kind of namespace for information to be saved/stored between them.
  • User ACL
  • TTL support

User experience

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

User can now use Redis as backend. You can connect it using Redis Standalone or Redis Cluster as shown in the example.

from aws_lambda_powertools.utilities.idempotency import (
    idempotent,
    RedisCachePersistenceLayer,
    IdempotencyConfig
)    

persistence_layer = RedisCachePersistenceLayer(host="192.168.68.112", port=6379, db_index=0)
config =  IdempotencyConfig(
    expires_after_seconds=1*60,  # 1 minutes
)

@idempotent(config=config, persistence_store=persistence_layer)
def lambda_handler(event, context):
    return {"message":"Hello"}

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.

@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Jun 23, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 23, 2023
@github-actions
Copy link
Contributor

No acknowledgement section found. Please make sure you used the template to open a PR and didn't remove the acknowledgment section. Check the template here: https://github.com/aws-powertools/powertools-lambda-python/blob/develop/.github/PULL_REQUEST_TEMPLATE.md#acknowledgment

@github-actions github-actions bot added do-not-merge need-license-agreement-acknowledge PRs that are missing acknowledgement section labels Jun 26, 2023
@github-actions
Copy link
Contributor

No related issues found. Please ensure there is an open issue related to this change to avoid significant delays or closure.

@github-actions github-actions bot added need-issue PRs that are missing related issues feature New feature or functionality labels Jun 26, 2023
@leandrodamascena
Copy link
Contributor

Hello @rubenfonseca! We are ready for you to review this PR a second time.

cc @roger-zhangg!

Copy link
Contributor

@rubenfonseca rubenfonseca left a comment

Choose a reason for hiding this comment

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

Just tiny things, super close now :D

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

  • JSON SerDe: waiting from @leandrodamascena to confirm that GET/SET + JSON SerDe is not slower than using HSETNX/HGET

This is an excellent point, Ruben! This PR had changes and optimizations throughout its development and one of the modifications was the migration of HSETNX/HGETALL to simplify with GET/SET and use JSON SerDe. To use HSETNX/HGETALL we have a change of concept in this implementation, as the traditional way is to make an HSETNX call for each key (in the dict) that we want to store in Redis as an object, after which we have to set ttl for all of them. But at this point, we made an optimization using the concept of pipelines in Redis, which optimizes latency, as it uses batch to write to Redis and reduces RTT, one of the main reasons for latency in Redis. But this makes the code more verbose and the business logic more complicated, in addition to the fact that the batch writing has to be transactional (this is by default), and in tests, this took even longer than using set/get with JSON SerDe, in some cases.

I recovered the old code, ran some tests, and saw no consistent difference in execution time between the two. My opinion is that we can continue with this implementation, and perhaps wait for feedback from some customers.

Cool! I'm 100% convinced this is the best approach. Thanks for the explanation!

@roger-zhangg
Copy link
Member Author

  • Orphan record Race condition without lock
graph TD;
    A(Existing orphan record in redis)--without lock-->A1;
    A1[Two Lambda invoke at same time]-->B1[Lambda handler1];
    B1-->B2[Fetch from Redis];
    B2-->B3[Handler1 got orphan record];
    B3-->B4[Handler1 overwrite orphan record];
    B4-->B5[Handler1 continue to execution];
    A1-->C1[Lambda handler2];
    C1-->C2[Fetch from Redis];
    C2-->C3[Handler2 got orphan record];
    C3-->C4[Handler2 overwrite orphan record];
    C4-->C5[Handler2 continue to execution];
    B5-->D(Lambda handler executed twice!);
    C5-->D;
Loading
  • Orphan record Race condition with lock
graph TD;
    A(Existing orphan record in redis)--with lock-->A1;
    A1[Two Lambda invoke at same time]-->B1[Lambda handler1];
    B1-->B2[Fetch from Redis];
    B2-->B3[Handler1 got orphan record];
    B3-->B4[Handler1 acquired lock];
    B4-->B5[Handler1 overwrite orphan record]
    B5-->B6[Handler1 continue to execution];
    A1-->C1[Lambda handler2];
    C1-->C2[Fetch from Redis];
    C2-->C3[Handler2 got orphan record];
    C3-->C4[Handler2 failed to acquire lock];
    C4-->C5[Handler2 wait and fetch from Redis];
    C5-->C6[Handler2 return without executing];
    B6-->D(Lambda handler executed only once!);
    C6-->D;
Loading

Copy link
Contributor

@rubenfonseca rubenfonseca 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 small things left

rubenfonseca
rubenfonseca previously approved these changes Jan 10, 2024
Copy link
Contributor

@rubenfonseca rubenfonseca left a comment

Choose a reason for hiding this comment

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

Awesome work everyone!

Copy link

sonarcloud bot commented Jan 10, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

6 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@leandrodamascena leandrodamascena self-requested a review January 10, 2024 18:27
Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

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

APPROVED!

@leandrodamascena leandrodamascena merged commit 1e7388c into aws-powertools:develop Jan 10, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commons dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation feature New feature or functionality github-actions Pull requests that update Github_actions code idempotency Idempotency utility internal Maintenance changes size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for MemoryDB/ElasticCache/Redis as Idempotency backend
7 participants