-
Notifications
You must be signed in to change notification settings - Fork 955
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
Add support for SSUBSCRIBE
#2813
Conversation
reactive commands implementation and additional tests
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2813 +/- ##
============================================
- Coverage 78.71% 77.66% -1.06%
- Complexity 6786 7195 +409
============================================
Files 508 539 +31
Lines 22765 24402 +1637
Branches 2446 2603 +157
============================================
+ Hits 17919 18951 +1032
- Misses 3717 4246 +529
- Partials 1129 1205 +76 ☔ View full report in Codecov by Sentry. |
src/main/java/io/lettuce/core/cluster/pubsub/RedisClusterShardedPubSubListener.java
Outdated
Show resolved
Hide resolved
src/main/java/io/lettuce/core/cluster/ClusterPubSubConnectionProvider.java
Outdated
Show resolved
Hide resolved
src/main/java/io/lettuce/core/cluster/PubSubClusterEndpoint.java
Outdated
Show resolved
Hide resolved
using default methods
9c7d5e0
to
e671633
Compare
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.
Awesome, IMHO this looks perfect! Thanks!
src/main/java/io/lettuce/core/cluster/pubsub/RedisClusterPubSubListener.java
Outdated
Show resolved
Hide resolved
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.
Generally makes a good impression. @since
tags are missing for newly introduced API. Please add these. Left also a few other comments.
Feel free to add me as reviewer via the PR reviewer functionality if you want me to review any future PR.
src/main/java/io/lettuce/core/cluster/pubsub/RedisClusterPubSubListener.java
Outdated
Show resolved
Hide resolved
...in/java/io/lettuce/core/cluster/pubsub/api/reactive/NodeSelectionPubSubReactiveCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/io/lettuce/core/cluster/pubsub/api/sync/NodeSelectionPubSubCommands.java
Outdated
Show resolved
Hide resolved
src/test/java/io/lettuce/core/cluster/pubsub/RedisClusterPubSubConnectionIntegrationTests.java
Outdated
Show resolved
Hide resolved
src/test/java/io/lettuce/core/cluster/pubsub/RedisClusterPubSubConnectionIntegrationTests.java
Outdated
Show resolved
Hide resolved
src/test/java/io/lettuce/core/pubsub/RedisPubSubAsyncCommandsImplUnitTests.java
Outdated
Show resolved
Hide resolved
…scribed delegates to subscribe - use of pubsubtestlistener instead of class level listener implementation - some format fixing
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. For the final merge, we typically squash all commits into one to keep one commit per ticket. That works well with tickets that introduce separate commands. @tishun do you want me to merge this one or do you want to merge it?
Please go ahead and thank you for the review! |
That's squashed, merged, and polished now. |
implementation for #2758
built on existing routing/command mechanism with adding 2 new interfaces (RedisShardedPubSubListener, RedisClusterShardedPubSubListener) to avoid breaking existing use cases.
need to add some more tests, including SPUBLISH command to check listeners are working properly.