Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
chore(v2): Split powertools-idempotency module to sub-modules and add redis implementation #1513
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
chore(v2): Split powertools-idempotency module to sub-modules and add redis implementation #1513
Changes from 19 commits
0ec7f4a
575e7a9
6382089
914359d
336cab3
dca1f0e
d71f9ac
a9ef2c4
c017948
bcbd956
9f44ebf
6da6aaf
e144be5
30722ba
7ff5708
d2e4efa
d00b5e2
b3e3d7d
0501b8f
0582f24
fce92df
0396be8
1a47aef
df7452f
4975dc9
65318c9
6cce077
0c1cc38
796029d
4d2c716
81417e1
a200e64
a82e4cd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Bit verbose - maybe "Using DynamoDB" is enough?
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.
"Using Redis"
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.
.... or some words that go to the "why is this a sensible default"
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.
I think this should be configurable; I don't think we should force people to put passwords in env vars. Long term it would be cool if we had IAM integration here, short term enough to be able to provide the password somehow, having retrieved it e.g. using the
SecretsManagerProvider
?(apologies if you can already do this - i haven't got to the code yet!)
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.
For IAM Integration, I am not sure, this would be an extra possibility, since Amazon ElasticCache is the only possible Redis deployment that people might use.
For SecretsManagerProvider, sure, I agree. I wanted to do that too. Since this is already a lot of implementation details in this PR, I felt I should put it for review, to discuss the approach, list findings and decide which of them are important to be included with this PR and which can be a in a next step.
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.
I agree, and don't think either of these things needs to happen here. The whole redis rabbit whole we started down for a separate impl to prove the abstraction out; we have that without this other bits!
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.
yes, having password as an environment variable is not a good practice. It should be something the user passes in the "connection string", so that he can retrieve it from secrets manager
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.
problem in this sentence...
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.
See how Python managed the connection: https://github.com/aws-powertools/powertools-lambda-python/pull/2567/files#diff-9aa1f6d28c9806e244b0bf7a7263e02efb02cf4986da20d1b8fc6bb931097884R48
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 Java folks! It's awesome that you're adding support for Redis in Idempotency!!
My 2 cents here. Two widespread use cases when using Redis are:
1 – Customers who want to use SSL connections to encrypt data in transit. I saw Jedis supports
SSL
parameter when building the client.2 - Customers who want to store data in a specific dbIndex - dbIndex is a logical database in Redis, the concept is something similar to schema. The default db_index is 0, but customers can store data in another db_index.
Do you think it is possible to add these 2 parameters in this implementation? For cases where the client wants to create a very specific connection with TLS, customizations, and others, we do the same as you: the customer can bring their own client.
Thanks 🚀
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.
Thanks @leandrodamascena for the feedback. I think it makes sense.
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.
With 0c1cc38 the option to provide a custom JedisClientConfig is added.
It can be used as described in the documentation
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.
not sure it is super useful, or explain how? but better would be to explain the security group config (if any), or any other networking config.
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.
probably deserve more explanations: SG of elasticache / SG of the function? what would be the link between the two SGs (port 6379, 6380 ?)
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.
link to JedisPooled?
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.
link at first usage, move it above.
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.
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:
@scottgerring wdyt ?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found also this extract here from redis.conf
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.
we cannot do this, sorry