Jedis 4

[sazzad16]

Description

• Changes for Jedis 4

  • Made the Connection class as the base of operations, instead of Jedis class
  • Introduced ConnectionProvider
  • Introduced CommandExecutor

• Added RedisJSON and RedisJSON 2 support

• Added RediSearch support

• Introduced JedisPooled, an easier to use JedisPool

• Introduced JedisSharding, replacing ShardedJedisPool and related classes

• Introduced ClusterPipeline and ShardedPipeline

• Introduced ReliableTransaction

[yangbodong22011]

Congratulations, a big change, first of all trust our unit tests.

[yangbodong22011]

We plan to no longer support ShardedJedis?

[sazzad16]

We plan to no longer support ShardedJedis?

@yangbodong22011 That's not a priority ATM. But I hope to re-implement it with parallel paradigm of cluster mode.

[codecov-commenter]

Codecov Report

Merging #2693 (d94f732) into master (253664f) will decrease coverage by 2.28%.
The diff coverage is 60.92%.

❗ Current head d94f732 differs from pull request most recent head 61197d6. Consider uploading reports for the commit 61197d6 to get more accurate results

@@             Coverage Diff              @@
##             master    #2693      +/-   ##
============================================
- Coverage     59.39%   57.11%   -2.29%     
- Complexity     2586     2818     +232     
============================================
  Files           133      169      +36     
  Lines         11995    10425    -1570     
  Branches        564      611      +47     
============================================
- Hits           7125     5954    -1171     
+ Misses         4640     4252     -388     
+ Partials        230      219      -11     

Impacted Files	Coverage Δ
src/main/java/redis/clients/jedis/Builder.java	100.00% <ø> (ø)
.../main/java/redis/clients/jedis/CommandObjects.java	72.11% <ø> (ø)
...c/main/java/redis/clients/jedis/GeoCoordinate.java	33.33% <ø> (ø)
src/main/java/redis/clients/jedis/Jedis.java	86.28% <ø> (-7.13%)	⬇️
...rc/main/java/redis/clients/jedis/JedisCluster.java	32.39% <ø> (+27.16%)	⬆️
...rc/main/java/redis/clients/jedis/JedisMonitor.java	100.00% <ø> (ø)
...main/java/redis/clients/jedis/JedisPoolConfig.java	100.00% <ø> (ø)
...ava/redis/clients/jedis/MultiNodePipelineBase.java	10.32% <ø> (ø)
src/main/java/redis/clients/jedis/Pipeline.java	10.50% <ø> (-80.53%)	⬇️
src/main/java/redis/clients/jedis/Response.java	80.00% <ø> (-14.29%)	⬇️
... and 201 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71cd5f1...61197d6. Read the comment docs.

[yangbodong22011]

@sazzad16 It seems that the syncAndReturnAll method of Pipeline has been removed. Is it planned or not yet supported? If it is not yet supported, I will submit a PR, including the syncAndReturnAll method of ClusterPipeline (in fact, there is already a test submission: yangbodong22011@16a29d6)

[sazzad16]

@yangbodong22011 I just wasn't sure about the importance of it. So I have not restored it.

[yangbodong22011]

@yangbodong22011 I just wasn't sure about the importance of it. So I have not restored it.

Such usage exists in our customers, so we need to restore it back. If you agree, I will introduce and test the recovery of Pipeline#syncAndReturnAll on this commit, so that we will have syncAndReturnAll methods on both ClusterPipeline and Pipeline.

[chayim]

I think we should take this opportunity to at least run the unit tests against the latest minor version of the included libraries. For instance junit 4.13.2 has been released since February. Let's update the poms.

It looks like code coverage decreases by 10% - so I think we should increase tests here as well, as part of merging.

I like the way commands are now split - it'll make implementation of missing commands much easier.

[chayim]

Some missing commands - there are others.

EXEC

• COMMAND GETKEYS
• STRALGO
• SYNC (only called on replicas)
• PSYNC (only called on replicas)

[sazzad16]

@chayim

• EXEC
  exec() methods are part Transaction classes.
• COMMAND GETKEYS
  We have a very old issue Is the redis COMMAND implemented? #793 for COMMAND. No one ever asked for COMMAND GETKEYS. Probably because in our current design we already know which are the keys.
• STRALGO
  We have STRALGO.
  PS: STRALGO is removed (not even friendly deprecation) from Redis and so should we. redis/redis@af74898
• SYNC & PSYNC (only called on replicas)
  "only called on replicas" - Jedis a client, not a replica.
  We used to have those but removed later because it didn't make sense to have those.

[sazzad16]

#2693 (comment)

I think we should increase tests here as well

@chayim Does it mean we are now allowed to take some time for that?

[lgtm-com]

This pull request introduces 4 alerts when merging 61197d6 into 71cd5f1 - view on LGTM.com

new alerts:

• 2 for Result of multiplication cast to wider type
• 2 for Array index out of bounds

[sazzad16]

We've finally merged the PR. Thank you @yangbodong22011, @AvitalFineRedis and @chayim.

[YNedderhoff]

Hey @sazzad16, I realise that this is just in RC state at the moment, but is there already any documentation about how to use Sharding, Pipelining, and especially pipelining with shards in version 4.0.0 so I can have a look already?

[sazzad16]

@YNedderhoff Sorry, there aren't enough documentation at this moment. We need both time and resource for that. Hopefully we'll get there.

In the mean time,

• For sharding, Jedis 4 will have JedisSharding. You can check the test files ShardedConnectionTest, ShardingTest.

  In brief, if previous code was

ShardedJedisPool pool = new ShardedJedisPool(...);
try (ShardedJedis jedis = pool.getResource()) {
  jedis.set(...);
}
try (ShardedJedis jedis = pool.getResource()) {
  jedis.get(...);
}
...
...

Then new code would be

JedisSharding jedis = new JedisSharding(...);
jedis.set(...);
jedis.get(...);
...
...

• For pipelining in shards, there will be ShardedPipeline. There are not enough friendly constructors. But it's mostly similar of using ClusterPipeline.

I hope, this is helpful for you.

[YNedderhoff]

Thank you, it is! Especially pleased to see that there will be a solution for sharded pipelining, as this PR had us all worried: #2488 (comment). Any ETA on that? We're basically stuck on 3.x until that is there in 4.x. Just asking for our planning I suppose.

[sazzad16]

@YNedderhoff Jedis-4.0.0-RC1 is already out. Final release could be next week.

[YNedderhoff]

Right - thank you!

[sazzad16]

@YNedderhoff FYI, in #2731 I have renamed JedisClusterPipelineTest to ClusterPipelineTest, added ShardedPipelineTest. Also added easier constructors for related classes.

[YNedderhoff]

Thanks for the update!

[dalei2019]

Thanks for the update! + 1
Cluster pipeline is a great feature!

[dalei2019]

@sazzad16 It seems that the syncAndReturnAll method of Pipeline has been removed. Is it planned or not yet supported? If it is not yet supported, I will submit a PR, including the syncAndReturnAll method of ClusterPipeline (in fact, there is already a test submission: yangbodong22011@16a29d6)

syncAndReturnAll is useful ！

[yangbodong22011]

@sazzad16 It seems that the syncAndReturnAll method of Pipeline has been removed. Is it planned or not yet supported? If it is not yet supported, I will submit a PR, including the syncAndReturnAll method of ClusterPipeline (in fact, there is already a test submission: yangbodong22011@16a29d6)

syncAndReturnAll is useful ！

@dalei2019 Do you want the syncAndReturnAll method of ClusterPipeline.class? (Note: Pipeline.class has been restore syncAndReturnAll)

[dalei2019]

syncAndReturnAll

Yes！It's very useful!
I look forward to using the pipeline function in cluster mode, just like single instance mode.

[sazzad16]

@dalei2019 Simply maintain a List<Response> and your code is same for both Pipeline and ClusterPipeline.

    List<Response> responses = new ArrayList<>();

    responses.add(pipeline.command(...));
    responses.add(pipeline.command(...));
    responses.add(pipeline.command(...));

    pipeline.sync();

    for (Response res : responses) {
      res.get();
    }

[MSPigl]

@sazzad16 Maybe this is the wrong place for this, however I'm seeing removals of functionality without documentation on replacement. I'm currently upgrading my team's codebase to Jedis 4. Currently, we're leveraging jedis.getClient(), and then using getHost() and getPort() from the returned object. I see that on Jedis 4, getClient() is just a passthrough to getConnection(), and I'm not seeing a way to get the host and port from a Jedis instance now. How would you recommend I get that data?

[YNedderhoff]

@YNedderhoff Sorry, there aren't enough documentation at this moment. We need both time and resource for that. Hopefully we'll get there.

I think that comment above by @sazzad16 sums it up. But yeah I agree, I'd have loved some migration documentation as well, ideally in time for release.

[sazzad16]

@MSPigl Only now that you have mentioned, we have just realized that getHost() and getPort() were not deprecated in Jedis 3.x. These were removed because these were related to setHost() and setPort() which were decided to be removed and deprecated.

Could you please describe you usage of getHost() and getPort()?
BTW, please continue this discussion in a new Discussion.
Thanks