Introduce JedisBroadcast to broadcast commands

[sazzad16]

No description provided.

[codecov-commenter]

Codecov Report

Base: 67.10% // Head: 66.98% // Decreases project coverage by -0.11% ⚠️

Coverage data is based on head (8165a8c) compared to base (ed57047).
Patch coverage: 50.52% of modified lines in pull request are covered.

❗ Current head 8165a8c differs from pull request most recent head 83537cc. Consider uploading reports for the commit 83537cc to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3194      +/-   ##
============================================
- Coverage     67.10%   66.98%   -0.12%     
- Complexity     4582     4600      +18     
============================================
  Files           248      251       +3     
  Lines         14751    14846      +95     
  Branches        917      920       +3     
============================================
+ Hits           9898     9944      +46     
- Misses         4459     4504      +45     
- Partials        394      398       +4     

Impacted Files	Coverage Δ
...is/clients/jedis/providers/ConnectionProvider.java	0.00% <0.00%> (ø)
...nts/jedis/providers/ShardedConnectionProvider.java	75.00% <0.00%> (-1.06%)	⬇️
.../main/java/redis/clients/jedis/JedisBroadcast.java	45.83% <45.83%> (ø)
...in/java/redis/clients/jedis/BroadcastResponse.java	54.54% <54.54%> (ø)
...rc/main/java/redis/clients/jedis/UnifiedJedis.java	75.26% <100.00%> (+0.02%)	⬆️
...nts/jedis/providers/ClusterConnectionProvider.java	78.57% <100.00%> (+0.38%)	⬆️
...ents/jedis/providers/PooledConnectionProvider.java	100.00% <100.00%> (ø)
...in/java/redis/clients/jedis/ConnectionFactory.java	63.26% <0.00%> (-4.09%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[yangbodong22011]

I prefer not to introduce JedisBroadcast, but directly modify the default behavior of commands such as cluster.scriptLoad and cluster.ftCreate (because the previous behavior is actually wrong, so we don't need to keep it).

Adding JedisBroadcast certainly won't cause a bug, but I think Jedis should keep it simple and quiet to fix the behavior.

[sazzad16]

@yangbodong22011 Are you okay with other things? I.e. changes in ConnectionProvider interface and implementations, method signatures, etc?

[yangbodong22011]

@yangbodong22011 Are you okay with other things? I.e. changes in ConnectionProvider interface and implementations, method signatures, etc?

That's OK, but I tend to avoid having PooledConnectionProvider provide a getConnectionMap() method.

[chayim]

@yangbodong22011 Are you okay with other things? I.e. changes in ConnectionProvider interface and implementations, method signatures, etc?

That's OK, but I tend to avoid having PooledConnectionProvider provide a getConnectionMap() method.

@yangbodong22011 why?

[yangbodong22011]

@yangbodong22011 Are you okay with other things? I.e. changes in ConnectionProvider interface and implementations, method signatures, etc?

That's OK, but I tend to avoid having PooledConnectionProvider provide a getConnectionMap() method.

@yangbodong22011 why?

PooledConnectionProvider usually corresponds to one node in the backend rather than multiple nodes. Exposing the interface(getConnectionMap) is only for the neatness of the code, but it will confuse the user.

[chayim]

PooledConnectionProvider usually corresponds to one node in the backend rather than multiple nodes. Exposing the interface(getConnectionMap) is only for the neatness of the code, but it will confuse the user.

I think that in the interest of consistency and quality, we should do this. If anything, a docstring can always be added in order to help users out.

[uglide]

Continuing the

I prefer not to introduce JedisBroadcast, but directly modify the default behavior of commands such as cluster.scriptLoad and cluster.ftCreate (because the previous behavior is actually wrong, so we don't need to keep it).

Continuing the @yangbodong22011 idea. @sazzad16 Why not override commands and make them broadcasted under the hood in the JedisCluster?

[sazzad16]

@uglide Please check #3211, it addresses your concerns. We can discuss if you have further concerns.

[uglide]

@sazzad16 I like the plug-in approach in this PR and don't like the new xxxBroadcast commands in the #3211 :)

Could you explain why we can't add broadcast commands to the JedisCluster instead? From my understanding, users are interested in "broadcasting" using JedisCluster in their codebases. Am I missing something?

[yangbodong22011]

@sazzad16 Now I vote #3194 instead of #3211 because:
1, scriptExists v.s. scriptExistsBroadcast I chose scriptExists.
2. I think the code of the #3194 plug-in is cleaner.

[sazzad16]

Could you explain why we can't add broadcast commands to the JedisCluster instead? From my understanding, users are interested in "broadcasting" using JedisCluster in their codebases. Am I missing something?

@uglide We can, but then...

We would miss these methods from other classes (UnifiedJedis, JedisPooled, JedisSharding). Let's discuss only with JedisPooled. We can add these methods in JedisPooled separately but then concerned methods cannot be (easily) switched between Redis standalone mode and Redis cluster mode. This was long ask from users which we just added in Jedis 4.0.0 and I am trying to keep common base between JedisCluster and JedisPooled.

[uglide]

@sazzad16 Thanks for explaining the context. This PR suits user needs better because it offers a "plug-in" approach, just like @yangbodong22011 mentioned in the comment above. If there are no objections, we should proceed with this PR and add more "broadcasted" commands.

[sazzad16]

@yangbodong22011 @uglide

• Added broadcasted() method, so the code can now be fluent. Naming is such for similarity with existing pipelined().
• Added broadcast() method, so the code can now be fluent.
• Added PING

Please review.

[yangbodong22011]

The current code and design look fine to me.

We have a few more commands worth adding to JedisBoardcast:

• keys
• flushDB
• functionXXX

@sazzad16 It's up to you to decide if you want to add it or it will be added later by another contributor（when they need）, I agree with both.