Skip to content

Add helpers to Jedis pool#4366

Merged
ggivo merged 6 commits into
redis:masterfrom
oscar-besga-panel:master
Nov 24, 2025
Merged

Add helpers to Jedis pool#4366
ggivo merged 6 commits into
redis:masterfrom
oscar-besga-panel:master

Conversation

@oscar-besga-panel

@oscar-besga-panel oscar-besga-panel commented Nov 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

This mininal chage of code will allow to use Jedis connections without making a try/catch

Before

    try (Jedis jedis = jedisPool.getResource()) {
      //some code
       jedis.setnx("hello", "there");
      //some code
    }

After

jedisPool.withResource( (jedis) -> {
      //some code
       jedis.setnx("hello", "there");
      //some code
 });

Cleaner in my opinion

@jit-ci

jit-ci Bot commented Nov 20, 2025

Copy link
Copy Markdown

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.

@oscar-besga-panel

oscar-besga-panel commented Nov 20, 2025

Copy link
Copy Markdown
Contributor Author

retest

@ggivo

ggivo commented Nov 21, 2025

Copy link
Copy Markdown
Collaborator

@oscar-besga-panel
Did you consider JedisPooled? It is available starting with Jedis 4 and helps to avoid a try-with-resources block for each command.

https://redis.io/docs/latest/develop/clients/jedis/connect/#connect-with-a-connection-pool

ggivo
ggivo requested changes Nov 21, 2025
View reviewed changes

@ggivo ggivo left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread src/main/java/redis/clients/jedis/JedisPool.java

public <K> K withJedisPoolGet(Function<Jedis, K> function) {
try (Jedis jedis = this.getResource()) {
return function.apply(jedis);

@ggivo ggivo Nov 21, 2025

Copy link
Copy Markdown
Collaborator

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

@oscar-besga-panel oscar-besga-panel Nov 21, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See bellow

@ggivo ggivo Nov 23, 2025

Copy link
Copy Markdown
Collaborator

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

Comment thread src/main/java/redis/clients/jedis/JedisPool.java Outdated
}
}

public void withJedisPoolDo(Consumer<Jedis> consumer) {

@ggivo ggivo Nov 21, 2025

Copy link
Copy Markdown
Collaborator

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

@oscar-besga-panel oscar-besga-panel Nov 21, 2025

Copy link
Copy Markdown
Contributor Author

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

@oscar-besga-panel

oscar-besga-panel commented Nov 21, 2025

Copy link
Copy Markdown
Contributor Author

@oscar-besga-panel Did you consider JedisPooled? It is available starting with Jedis 4 and helps to avoid a try-with-resources block for each command.

https://redis.io/docs/latest/develop/clients/jedis/connect/#connect-with-a-connection-pool

Yes, but I use JedisPool in my library for now.
(considering change as is Legacy but not deprecated)

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.
Well you can use MULTI/EXEC or pipeline, thougth

@oscar-besga-panel

oscar-besga-panel commented Nov 21, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.
If it is to be done, in another PR

@ggivo
Clean up + unit test
9a0b4b8 
   - Add unit test to verify connection is returned to the pool
@ggivo

ggivo commented Nov 23, 2025

Copy link
Copy Markdown
Collaborator

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. If it is to be done, in another PR

I wouldn't go that way. JedisPooled is based on UnifiedJedis. Let's discuss this separately.

@ggivo

ggivo commented Nov 23, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

@oscar-besga-panel
Did a bit of unit test clean up; other than that, the change looks good to me. Let me know if you plan more changes or we are good to go and merge it once test's are green

@oscar-besga-panel

oscar-besga-panel commented Nov 23, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

It looks good to me, merge when appropiate.
edit: typo

@ggivo ggivo added this to the 7.2.0 milestone Nov 24, 2025
ggivo
ggivo approved these changes Nov 24, 2025
View reviewed changes

@ggivo ggivo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@ggivo ggivo merged commit 0a92e5d into redis:master Nov 24, 2025
14 of 15 checks passed
@oscar-besga-panel

oscar-besga-panel commented Nov 24, 2025

Copy link
Copy Markdown
Contributor Author

Thanks

@ggivo

ggivo commented Nov 24, 2025

Copy link
Copy Markdown
Collaborator

@oscar-besga-panel

Thanks a lot for the PR and the cleanup — appreciate the time and effort you put into this.Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ggivo ggivo ggivo approved these changes

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

7.2.0

Development

Successfully merging this pull request may close these issues.

2 participants

@oscar-besga-panel @ggivo