-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
[STORM-3680] Upgrade Jedis Library #3314
Conversation
* @return instance which implements JedisCommands | ||
*/ | ||
JedisCommands getInstance(); | ||
public interface JedisCommandsContainer extends Closeable { |
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.
Likely JedisCommandsAdapter
is a better name here as we are providing a single unified interface ontop of both JedisPool
and JedisCluster
as Jedis removed the JedisCommands
interface from covering both. See redis/jedis#1932 for more details on why this required
@HeartSaVioR since it looks like you were the last to make any significant changes to this module, do you mind taking a look and letting me your thoughts on this, as well if using TestContainers is an appropriate way to add test coverage for this? Thanks! |
95ed447
to
d6f4143
Compare
oh snap, is this finally get a review? |
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.
Hey @Crim
thanks for the PR and sorry, that it was pending for around 3 years. We revived Storm recently from the dead and are trying to maintain some of the 3rd party dependencies.
While looking through the open PRs, I saw this PR and thought it is time to update the Jedis version anyway.
I am fine with testcontainers as we introduced it for the stale (but now updated) elasticsearch module, too.
Regarding the naming: I would take it "as is" for know. We can change it later :)
What is the purpose of the change
To support future improvements to the storm-redis package, lets upgrade its underlying library (Jedis) it uses to communicate with Redis.
Upgrading will clear the way for STORM-3665 as Stream support in the Jedis library was not added until version 3.x
How was the change tested / Note Question
As no previous test coverage existed for the bolts provided by
storm-redis
I've started to add integration tests using TestContainers which depends on Docker being available and running to provide Redis instances. Let me know if this is an acceptable way to provide test coverage and I will build out tests for the remaining bolts.