chore(v2): Split powertools-idempotency module to sub-modules and add redis implementation #1513
Conversation
💾 Artifacts Size Report
|
|
This started with PR #1504. The branch was moved from the forked repository eldimi:chore/v2-split-idempotency-mod to the aws-powertools:chore/v2-split-idempotency-mod. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## v2 #1513 +/- ##
=====================================
Coverage ? 89.79%
Complexity ? 406
=====================================
Files ? 44
Lines ? 1274
Branches ? 165
=====================================
Hits ? 1144
Misses ? 88
Partials ? 42 ☔ View full report in Codecov by Sentry. |
jeromevdl
changed the title
| import software.amazon.lambda.powertools.idempotency.internal.cache.LRUCache; | ||
|
|
||
| import java.time.Duration; |
There was a problem hiding this comment.
I think you don't have checkstyle applied.
There was a problem hiding this comment.
It should be fixed now. I think it is because we only format uncommitted and if I forget to format before commit, I am missing some files..
...src/main/java/software/amazon/lambda/powertools/idempotency/internal/IdempotencyHandler.java
Outdated
Show resolved
Hide resolved
|
Verified that it is also compatible with Amazon MemoryDB for Redis and ElastiCache Serverless. Also, used ElastiCache Serverless in the e2e test. |
docs/utilities/idempotency.md
Outdated
|
|
||
| !!! warning "Warning: Large responses with Redis persistence layer" | ||
| When using this utility with Redis your function's responses must be smaller than 512MB. | ||
| Persisting larger items cannot might cause exceptions. |
There was a problem hiding this comment.
problem in this sentence...
docs/utilities/idempotency.md
Outdated
| ```java hl_lines="2-11 13 18" | ||
| public App() { | ||
| JedisConfig jedisConfig = JedisConfig.Builder.builder() | ||
| .withHost(redisCluster.getHost()) |
There was a problem hiding this comment.
where does redisCluster come from? Would be great to have the full detail here as it's not super easy...
There was a problem hiding this comment.
c&p from code. I will replace that
docs/utilities/idempotency.md
Outdated
| .build()) | ||
| .build(); | ||
|
|
||
| JedisPooled jedisPooled = new JedisPooled(new HostAndPort("host",6789), jedisConfig) |
There was a problem hiding this comment.
we need to define the host / port twice ? (line 973 and here)
| } | ||
| ``` | ||
|
|
||
| !!! info "Default configuration is the following:" |
There was a problem hiding this comment.
What do you mean by "default configuration", it is what we use as default ? If that's the case, does it make sense? Not sure anyone is using the user "default" with no password...
I think we should ask for all the required fields to create a JedisClient ourself:
- host (mandatory)
- port (default: 6379)
- user (mandatory)
- password (mandatory)
- ssl (default: false)
- database ?
- something else ?
@scottgerring wdyt ?
There was a problem hiding this comment.
Default here means, as provided by the jedis client default config.
In Redis before version 6, you could only require password for auth .
Also, by default all access is unathenticated and a user with name "default" is created. So, by default (that is the redis default), you can execute commands either without any user/pass or with user:"default" and an empty pass. The user "default" is created even after redis version 6, for backwards compatibility purposes.
See ACL redis doc.
For these reasons, I am not sure we should make them mandatory.
Actually I have to update the powertools documentation, since user/pass, by default, in JedisClient are null.
There was a problem hiding this comment.
Found also this extract here from redis.conf
The special username "default" is used for new connections. If this user
has the "nopass" rule, then new connections will be immediately authenticated
as the "default" user without the need of any password provided via the
AUTH command. Otherwise if the "default" user is not flagged with "nopass"
the connections will start in not authenticated state, and will require
AUTH (or the HELLO command AUTH option) in order to be authenticated and
start to work.
...ools-e2e-tests/src/test/java/software/amazon/lambda/powertools/testutils/Infrastructure.java
Outdated
Show resolved
Hide resolved
...ools-e2e-tests/src/test/java/software/amazon/lambda/powertools/testutils/Infrastructure.java
Outdated
Show resolved
Hide resolved
| Function function = Function.Builder | ||
| .create(stack, functionName) | ||
|
|
||
| final SecurityGroup lambdaSecurityGroup = SecurityGroup.Builder.create(this.stack, "Lambda-SG") |
There was a problem hiding this comment.
this should be done only if (isRedisDeployment)
| List<String> subnets = vpc.getPublicSubnets().stream().map(subnet -> | ||
| subnet.getSubnetId()).collect(Collectors.toList()); | ||
|
|
||
| securityGroup = SecurityGroup.Builder.create(stack, "ElastiCache-SG-" + stackName) |
There was a problem hiding this comment.
| securityGroup = SecurityGroup.Builder.create(stack, "ElastiCache-SG-" + stackName) | |
| redisSecurityGroup = SecurityGroup.Builder.create(stack, "ElastiCache-SG-" + stackName) |
| private String ddbStreamsTableName; | ||
| private String functionName; | ||
| private Object cfnTemplate; | ||
| private String cfnAssetDirectory; | ||
| private SubnetSelection subnetSelection; | ||
| private CfnSubnetGroup cfnSubnetGroup; | ||
| private SecurityGroup securityGroup; |
There was a problem hiding this comment.
| private SecurityGroup securityGroup; | |
| private SecurityGroup redisSecurityGroup; |
| .allowAllOutbound(true) | ||
| .description("Lambda SecurityGroup") | ||
| .build(); | ||
| securityGroup.addIngressRule(Peer.securityGroupId(lambdaSecurityGroup.getSecurityGroupId()), Port.tcp(6379), |
There was a problem hiding this comment.
| securityGroup.addIngressRule(Peer.securityGroupId(lambdaSecurityGroup.getSecurityGroupId()), Port.tcp(6379), | |
| redisSecurityGroup.addIngressRule(Peer.securityGroupId(lambdaSecurityGroup.getSecurityGroupId()), Port.tcp(6379), |
| throw new IllegalStateException("Utility class"); | ||
| } | ||
|
|
||
| public static final String REDIS_CLUSTER_MODE = "REDIS_CLUSTER_MODE"; |
There was a problem hiding this comment.
should we move this to the RedisPersistenceStore config ?
to have everything at the same place:
RedisPersistenceStore.builder()
.withHost(System.getenv("REDIS_HOST"))
.withPort(System.getenv("REDIS_PORT")) // optional, default 6379
.withUsername(secretsProvider.getValue("REDIS_USERNAME"))
.withPassword(secretsProvider.getValue("REDIS_PASSWORD"))
.withSsl(true) // optional, default false ? or should it be true as default
.withClusterMode(true) // optional, default false
.build();
There was a problem hiding this comment.
what is this lua code for?
There was a problem hiding this comment.
as mentioned above, I think we should hide this, but maybe I miss something... @scottgerring
Co-authored-by: Jérôme Van Der Linden <[email protected]>
|
|
Closing for now in favor of #1559. We can pick up the redis changes here in a later branch, but let's get the base module split into v2! |
Issue #1467
This started with PR #1504. The branch was moved from the forked repository eldimi:chore/v2-split-idempotency-mod to the aws-powertools:chore/v2-split-idempotency-mod.
Reference the previous PR #1504, for older discussions.
PR #1504 will be closed without merge now.
Description of changes:
The idempotency module is split to sub-modules so that the various implementations (currently only dynamodb), are provided as pluggable extensions
Also, a Redis implementation is provided. The Redis deployment can be either standalone or cluster and the authentication supported is username/password. Certificate authentication is not included.
The e2e tests are implemented using an ElastiCache standalone deployment. This means also VPC, segurity groups and subnets are created to support it.
We can also use https://upstash.com/ which will basically mean, set-up an account and a server in upstash.com which should be maintained. This makes test execution more quick, but has the drawback of mainting an external service - and not maintaining a test case that consistency testing on Amazon ElastiCache.
One thought could be to have both tests and enable/disbale them with a flag according to needs.
Amazon ElastiCache Serveless doesn't seem to be supported with the current implementation, most probably because it gets created by default with encryption in transit and there is no option to modify it.
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.