feat(jedis-compatibility): add pub sub commands jedis layer#5286
feat(jedis-compatibility): add pub sub commands jedis layer#5286prashanna-frsh wants to merge 26 commits intovalkey-io:mainfrom
Conversation
|
Hi @prashanna-frsh, it looks like some of your commits are not verified. We have a policy that all commits needs to have signing and signoff. We can help review your PR after these are being taken care of. |
…bChannels, pubsubNumPat, pubsubNumSub) Signed-off-by: prashanna-frsh <prashanna.rajendran@freshworks.com>
… of customCommand Signed-off-by: prashanna-frsh <prashanna.rajendran@freshworks.com>
…mands Signed-off-by: prashanna-frsh <prashanna.rajendran@freshworks.com>
…cription APIs Update subscribe/unsubscribe methods in Jedis compatibility layer to use GLIDE's dynamic subscription APIs from PR valkey-io#5269 instead of customCommand(). Signed-off-by: prashanna-frsh <prashanna.rajendran@freshworks.com>
f025365 to
6ef11fb
Compare
Signed the commits |
…Sub tests Signed-off-by: prashanna-frsh <prashanna.rajendran@freshworks.com>
currantw
left a comment
There was a problem hiding this comment.
Thanks a lot for your work on this @prashanna-frsh !
I have reviewed Jedis.java so far and added some comments. Will try to finish reviewing the remaining files shortly.
java/jedis-compatibility/src/main/java/redis/clients/jedis/Jedis.java
Outdated
Show resolved
Hide resolved
java/jedis-compatibility/src/main/java/redis/clients/jedis/Jedis.java
Outdated
Show resolved
Hide resolved
java/jedis-compatibility/src/main/java/redis/clients/jedis/Jedis.java
Outdated
Show resolved
Hide resolved
java/jedis-compatibility/src/main/java/redis/clients/jedis/Jedis.java
Outdated
Show resolved
Hide resolved
java/jedis-compatibility/src/main/java/redis/clients/jedis/Jedis.java
Outdated
Show resolved
Hide resolved
java/jedis-compatibility/src/main/java/redis/clients/jedis/Jedis.java
Outdated
Show resolved
Hide resolved
java/jedis-compatibility/src/main/java/redis/clients/jedis/Jedis.java
Outdated
Show resolved
Hide resolved
Signed-off-by: prashanna-frsh <prashanna.rajendran@freshworks.com>
java/integTest/src/test/java/compatibility/jedis/JedisTest.java
Outdated
Show resolved
Hide resolved
java/jedis-compatibility/src/main/java/redis/clients/jedis/Jedis.java
Outdated
Show resolved
Hide resolved
java/jedis-compatibility/src/main/java/redis/clients/jedis/Jedis.java
Outdated
Show resolved
Hide resolved
| List<String> channelsWithPattern = jedis.pubsubChannels(channel); | ||
| assertNotNull(channelsWithPattern, "PUBSUB CHANNELS pattern should return a list"); |
There was a problem hiding this comment.
We should be able to assert here that is just returns a list with the channel right (not just any list)? Similar with some of the other tests below, I think we can go a bit more specific?
java/jedis-compatibility/src/test/java/redis/clients/jedis/JedisWrapperTest.java
Outdated
Show resolved
Hide resolved
java/jedis-compatibility/src/test/java/redis/clients/jedis/JedisPubSubCommandsTest.java
Outdated
Show resolved
Hide resolved
| List<byte[]> channelsBinary = jedis.pubsubChannels(channelBytes); | ||
| assertNotNull(channelsBinary, "PUBSUB CHANNELS binary should return a list"); | ||
|
|
||
| Map<byte[], Long> numSubBinary = jedis.pubsubNumSub(channelBytes); |
There was a problem hiding this comment.
We need to examine the contents of the map.
There was a problem hiding this comment.
@prashanna-frsh please address this comment. We need to assert that the values in the map are what we expect.
java/integTest/src/test/java/compatibility/jedis/JedisTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: prashanna-frsh <prashanna.rajendran@freshworks.com>
Signed-off-by: prashanna-frsh <prashanna.rajendran@freshworks.com>
Signed-off-by: prashanna-frsh <prashanna.rajendran@freshworks.com>
| * | ||
| * @return the number of unique patterns | ||
| */ | ||
| public long pubsubNumPat() { |
There was a problem hiding this comment.
This should return a java.lang.Long rather than primitive long. Please correct the signature and check over the signatures for other methods.
There was a problem hiding this comment.
addressed it
| * @param channels channel names | ||
| * @return map of channel name to subscriber count | ||
| */ | ||
| public Map<byte[], Long> pubsubNumSub(final byte[]... channels) { |
There was a problem hiding this comment.
Is the binary overload needed? I don't see it in the 5.1.5 Jedis API
There was a problem hiding this comment.
addressed it
| * @param pattern glob-style pattern (pass null or empty array for all channels) | ||
| * @return list of channel names | ||
| */ | ||
| public List<byte[]> pubsubChannels(final byte[] pattern) { |
There was a problem hiding this comment.
Please verify if the binary overload is required.
There was a problem hiding this comment.
addressed it
| * @return a List of byte arrays | ||
| */ | ||
| private static List<byte[]> convertGlideStringArrayToByteArrayList(GlideString[] glideStrings) { | ||
| List<byte[]> result = new ArrayList<>(); |
There was a problem hiding this comment.
Nit: Let's specify the capacity of the list since we know the length of the input.
There was a problem hiding this comment.
addressed it
| - ✅ Key operations (DEL, EXISTS, EXPIRE, TTL) | ||
| - ✅ Connection commands (PING, SELECT) | ||
| - ✅ ACL commands (ACL LIST, ACL GETUSER, ACL SETUSER, ACL DELUSER, ACL CAT, ACL GENPASS, ACL LOG, ACL LOG RESET, ACL WHOAMI, ACL USERS, ACL SAVE, ACL LOAD, ACL DRYRUN) | ||
| - ✅ Pub/Sub (PUBLISH, PUBSUB CHANNELS, PUBSUB NUMSUB, PUBSUB NUMPAT). `publish()` returns `0` instead of the actual subscriber count due to GLIDE API limitations (see [issue #5354](https://github.com/valkey-io/valkey-glide/issues/5354)). For subscription management and message handling, use GLIDE's native `StandaloneSubscriptionConfiguration` or `ClusterSubscriptionConfiguration` at client creation time. |
There was a problem hiding this comment.
This comment should be changed because Java supports dynamic pubsub now. They don't have to use subscription configuration at client creation time. Let's just tell the user to use Glide's native pubsub features.
There was a problem hiding this comment.
addressed it
| - **Pipelining**: Jedis pipelining functionality unavailable | ||
| - **Pub/Sub**: Redis publish/subscribe not implemented | ||
| - ✅ **Lua scripting**: Full support for EVAL/EVALSHA, SCRIPT management, and Valkey Functions (FCALL/FUNCTION *) | ||
| - **Pub/Sub with JedisPubSub callbacks**: Jedis-style `JedisPubSub` callback listeners are not supported. For PubSub functionality with message handling, configure GLIDE's native `StandaloneSubscriptionConfiguration` or `ClusterSubscriptionConfiguration` at client creation time to receive messages via callbacks or message queues. The compatibility layer only provides `publish()` and `pubsub*()` inspection commands. |
There was a problem hiding this comment.
Similar comment to above. Users should use Glide's native pubsub functionality rather than subscription configuration specifically.
There was a problem hiding this comment.
addressed it
Signed-off-by: prashanna-frsh <prashanna.rajendran@freshworks.com>
Signed-off-by: prashanna-frsh <prashanna.rajendran@freshworks.com>
|
Hi @prashanna-frsh, It looks like there are still a few comments that haven’t been replied to or addressed yet. Would you be able to revisit those and either resolve them or leave a note indicating whether they’ve been addressed, and if so, in which commit? That would be very helpful for tracking and review purposes. Thank you! |
Signed-off-by: prashanna-frsh <prashanna.rajendran@freshworks.com>
Feedback Status UpdateI've reviewed all outstanding feedback and addressed the remaining items: ✅ Addressed
📝 Notes on Other Feedback
All feedback items from the review have been addressed or clarified. |
…ub callback support Add reference to issue valkey-io#5469 in migration guide to track planned support for Jedis-style JedisPubSub callback listeners. Addresses feedback from PR valkey-io#5286 review. Signed-off-by: prashanna-frsh <prashanna.rajendran@freshworks.com> Made-with: Cursor
…ub callback support Add reference to issue valkey-io#5469 in migration guide to track planned support for Jedis-style JedisPubSub callback listeners. Addresses feedback from PR valkey-io#5286 review. Signed-off-by: prashanna-frsh <prashanna.rajendran@freshworks.com> Made-with: Cursor Signed-off-by: prashanna-frsh <prashanna.rajendran@freshworks.com> Made-with: Cursor
faced02 to
66bbbeb
Compare
|
Thanks for your work on this @prashanna-frsh ! I have been quite busy. Will do my best to take another look in the next 2 days. |
currantw
left a comment
There was a problem hiding this comment.
Thanks for the update and sorry for the review delay!
Some thoughts:
- Does it also make sense to add these methods for
JedisClusterin this pull request as well? - I think we are still missing the test coverage we would need to be able to merge this.
java/benchmarks/src/main/java/glide/benchmarks/clients/jedis/JedisClient.java
Show resolved
Hide resolved
java/jedis-compatibility/src/main/java/redis/clients/jedis/Jedis.java
Outdated
Show resolved
Hide resolved
java/jedis-compatibility/src/main/java/redis/clients/jedis/Jedis.java
Outdated
Show resolved
Hide resolved
Signed-off-by: prashanna-frsh <prashanna.rajendran@freshworks.com>
Signed-off-by: prashanna-frsh <prashanna.rajendran@freshworks.com>
|
Added pubsub_introspection_with_active_subscription() for testing it |
currantw
left a comment
There was a problem hiding this comment.
Looks good! Just a few minor comments to address.
| * @see <a href="https://valkey.io/commands/publish/">valkey.io</a> for details. | ||
| * @since Valkey 1.0.0 | ||
| */ | ||
| public long publish(String channel, String message) { |
There was a problem hiding this comment.
I think this need to return Long for Jedis compatibility? Same for the binary publish method.
| * @param pattern glob-style pattern | ||
| * @return list of channel names | ||
| */ | ||
| public List<String> pubsubChannels(String pattern) { |
There was a problem hiding this comment.
Nit. Would it be possible to group all the pub/sub methods together. Will be tough to read for future developers if they're sprinkled through this file. 🤷
| public List<String> pubsubChannels() { | ||
| return executeCommandWithGlide( | ||
| "PUBSUB CHANNELS", | ||
| () -> { | ||
| String[] arr = glideClient.pubsubChannels().get(); | ||
| return arr != null ? Arrays.asList(arr) : Collections.emptyList(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
I do not believe that pubsubChannels will ever return null, so this can be simplified. Same for the other methods with similar implementations.
| public List<String> pubsubChannels() { | |
| return executeCommandWithGlide( | |
| "PUBSUB CHANNELS", | |
| () -> { | |
| String[] arr = glideClient.pubsubChannels().get(); | |
| return arr != null ? Arrays.asList(arr) : Collections.emptyList(); | |
| }); | |
| } | |
| public List<String> pubsubChannels() { | |
| return executeCommandWithGlide( | |
| "PUBSUB CHANNELS", | |
| () -> Arrays.asList(glideClient.pubsubChannels().get())); | |
| } |
| - ✅ Wait operations (WAIT, WAITAOF) | ||
| - ✅ Object introspection (OBJECT ENCODING, OBJECT FREQ, OBJECT IDLETIME, OBJECT REFCOUNT) | ||
| - ✅ Geospatial operations (GEOADD, GEOPOS, GEODIST, GEOHASH, GEOSEARCH, GEOSEARCHSTORE) | ||
| - ✅ Pub/Sub (PUBLISH, PUBSUB CHANNELS, PUBSUB NUMSUB, PUBSUB NUMPAT). `publish()` returns `0` instead of the actual subscriber count due to GLIDE API limitations (see [issue #5354](https://github.com/valkey-io/valkey-glide/issues/5354)). For subscription management and message handling, use GLIDE's native dynamic pubsub APIs for subscribing to channels and handling messages at runtime. |
There was a problem hiding this comment.
GLIDE's native dynamic pubsub APIs
Not sure exactly what this refers to? Are those APIs implemented yet?
| * channel appears and subscriber count is correct. | ||
| */ | ||
| @Test | ||
| void pubsub_introspection_with_active_subscription() throws Exception { |
There was a problem hiding this comment.
This looks much better! Can we please split it out into separate tests for each method and each test case?
As an example, here are the tests that I implemented for the Valkey GLIDE C# client's pub/sub introspection commands:
PubSubChannelsAsync_ReturnsEmptyPubSubChannelsAsync_WithActiveSubscription_ReturnsChannelPubSubChannelsAsync_WithPattern_ReturnsMatchingChannelsPubSubNumSubAsync_WithNoSubscribers_ReturnsZeroCountsPubSubNumSubAsync_WithSubscribers_ReturnsChannelCountsPubSubNumPatAsync_WithNoPatternSubscriptions_ReturnsZeroPubSubNumPatAsync_WithPatternSubscriptions_ReturnsPatternCount
We can certainly re-use the same GlideClient between test methods if convenient, but I think it is best to have separate test methods? Similarly, having separate tests for publish would be useful.
Summary
This PR adds comprehensive PubSub (Publish/Subscribe) command support to the Jedis compatibility layer, enabling applications to migrate from Jedis to Valkey GLIDE with full PubSub API compatibility. The implementation includes publishing commands, subscription/unsubscription commands, and channel introspection commands with both String and binary variants.
Issue link
This Pull Request is linked to issue: #5285
Features / Behaviour Changes
Publishing & Introspection Commands:
publish()- Publish messages to channels (returns 0 as GLIDE doesn't expose subscriber count)pubsubChannels()- List active channels with optional pattern filteringpubsubNumPat()- Get count of unique pattern subscriptionspubsubNumSub()- Get subscriber counts for specific channelsSubscription Commands:
subscribe()/unsubscribe()- Subscribe/unsubscribe to exact channelspsubscribe()/punsubscribe()- Subscribe/unsubscribe to channel patterns (glob-style)ssubscribe()/sunsubscribe()- Subscribe/unsubscribe to sharded channels (cluster mode, Valkey 7.0+)All commands include both String and binary (byte[]) variants for full Jedis API compatibility. Total of 16 new public methods added to the
Jedisclass.Implementation
Publishing commands (lines 6996-7136 in Jedis.java):
glideClient.publish(),glideClient.pubsubChannels(), etc.)executeCommandWithGlide()pattern for consistent error handlingpublish()returns0Las GLIDE doesn't provide subscriber count in responseSubscription commands (lines 7022-7315 in Jedis.java):
glideClient.customCommand()to send subscription commands to servervoidas per Jedis API contractKey code highlights:
byte[]toGlideStringfor GLIDE compatibilityDocumentation updates:
compatibility-layer-migration-guide.mdwith complete PubSub command listLimitations
Subscription message handling:
The
subscribe(),psubscribe(), andssubscribe()methods send subscription commands to the server but do not handle incoming PubSub messages. This is a deliberate design choice aligned with GLIDE's architecture.For full PubSub functionality with message callbacks, applications should use GLIDE's native subscription configuration:
Other limitations:
publish()returns0instead of actual subscriber count (GLIDE API limitation)ssubscribe()/sunsubscribe()only work in cluster mode (will fail on standalone)JedisPubSubcallback listeners are not supportedTesting
Unit tests (
JedisPubSubCommandsTest.java):Integration tests (
JedisPubSubIntegrationTest.java):Manual testing:
Test execution:
Checklist
Before submitting the PR make sure the following are checked:
make *-linttargets) and Prettier has been run (make prettier-fix).