-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Add helpers to Jedis pool #4366
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
Conversation
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
|
retest |
|
@oscar-besga-panel https://redis.io/docs/latest/develop/clients/jedis/connect/#connect-with-a-connection-pool |
ggivo
left a comment
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.
Looks good in general,
needs unit tests to verify the resource is returned to the pool, on success, on error.
|
|
||
| public <K> K withJedisPoolGet(Function<Jedis, K> function) { | ||
| try (Jedis jedis = this.getResource()) { | ||
| return function.apply(jedis); |
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 suggest :
Rename withJedisPoolGet → withResource
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 bellow
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.
make sense. let's keep withResource/withResourceGet
| } | ||
| } | ||
|
|
||
| public void withJedisPoolDo(Consumer<Jedis> consumer) { |
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 suggest :
Rename withJedisPoolDo → withResource
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.
The problem is if the two methods are renamed equally this won't compile
withResource(jedis -> jedis.set("c"));
String x = withResource(jedis -> jedis.get("c")); as it will trigger ambiguous method call in both lines.
Even if I like your idea, Java won't allow
(if you use a full body lambda it will make it, but that will reducefuture uses of the methods)
I'll suggest withResource and withResourceGet
Yes, but I use JedisPool in my library for now. JedisPooled is 99% the times better, except if you want to send multiple commands. then it will be getting/closing a pooled connection for every one. |
|
I'm considering extending JedisPooled to be compatible with JedisPool by making it return a Jedis object... but not for now; and I know it has serious challenges. |
- Add unit test to verify connection is returned to the pool
I wouldn't go that way. JedisPooled is based on |
|
@oscar-besga-panel |
|
It looks good to me, merge when appropiate. |
ggivo
left a comment
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.
LGTM
Add helpers to Jedis pool (redis#4366)
|
Thanks |
|
Thanks a lot for the PR and the cleanup — appreciate the time and effort you put into this.Thanks for the contribution! |
This mininal chage of code will allow to use Jedis connections without making a try/catch
Before
After
Cleaner in my opinion