Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for SSUBSCRIBE #2813

Closed
wants to merge 4 commits into from
Closed

Add support for SSUBSCRIBE #2813

wants to merge 4 commits into from

Conversation

atakavci
Copy link
Collaborator

@atakavci atakavci commented Apr 2, 2024

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.

  • You have read the contribution guidelines.
  • You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

@atakavci
implementation of ssubscribe
948d017 
reactive commands implementation and additional tests
@codecov Codecov
Copy link

codecov bot commented Apr 2, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 79.31034% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 77.66%. Comparing base (43843bf) to head (5616087).
Report is 246 commits behind head on main.

Files Patch % Lines
...io/lettuce/core/cluster/PubSubClusterEndpoint.java 60.00% 2 Missing ⚠️
...ore/cluster/pubsub/RedisClusterPubSubListener.java 0.00% 2 Missing ⚠️
...core/cluster/pubsub/RedisClusterPubSubAdapter.java 0.00% 1 Missing ⚠️
...ava/io/lettuce/core/pubsub/RedisPubSubAdapter.java 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@atakavci atakavci force-pushed the ali/lettuce/issue-2758 branch from 9c7d5e0 to e671633 Compare April 3, 2024 09:58
@atakavci atakavci mentioned this pull request Apr 3, 2024
Closed
4 tasks
tishun
tishun approved these changes Apr 3, 2024
View reviewed changes
Copy link
Collaborator

@tishun tishun left a 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
mp911de
mp911de reviewed Apr 4, 2024
View reviewed changes
Copy link
Collaborator

@mp911de mp911de left a 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/main/java/io/lettuce/core/pubsub/RedisPubSubListener.java Outdated Show resolved Hide resolved
src/main/java/io/lettuce/core/pubsub/RedisPubSubListener.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/PubSubCommandBuilderUnitTests.java Show resolved Hide resolved
src/test/java/io/lettuce/core/pubsub/PubSubCommandTest.java Outdated Show resolved Hide resolved
src/test/java/io/lettuce/core/pubsub/RedisPubSubAsyncCommandsImplUnitTests.java Outdated Show resolved Hide resolved
atakavci added 2 commits April 5, 2024 12:45
@atakavci
- RedisPubSubListener.ssubscribed and RedisClusterPubSubListener.ssub…
b26e244 
…scribed delegates to subscribe

- use of pubsubtestlistener instead of class level listener implementation
- some format fixing
@atakavci
migrate to assertj
5616087
@mp911de mp911de self-requested a review April 5, 2024 12:15
mp911de
mp911de approved these changes Apr 5, 2024
View reviewed changes
Copy link
Collaborator

@mp911de mp911de left a 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?

@tishun
Copy link
Collaborator

tishun commented Apr 5, 2024

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!

@mp911de mp911de changed the title implementation of SSUBSCRIBE Implementation of SSUBSCRIBE Apr 8, 2024
@mp911de mp911de linked an issue Apr 8, 2024 that may be closed by this pull request
Add support for SSUBSCRIBE #2758
Closed
@mp911de mp911de changed the title Implementation of SSUBSCRIBE Add support for SSUBSCRIBE Apr 8, 2024
@mp911de mp911de added the type: feature A new feature label Apr 8, 2024
mp911de pushed a commit that referenced this pull request Apr 8, 2024
@atakavci @mp911de
Add support for SSUBSCRIBE #2758
1457486 
Original pull request: #2813
mp911de added a commit that referenced this pull request Apr 8, 2024
@mp911de
Polishing #2758
c121af5 
Introduce convenience method to clear listener state.

Original pull request: #2813
@mp911de
Copy link
Collaborator

mp911de commented Apr 8, 2024

That's squashed, merged, and polished now.

@mp911de mp911de closed this Apr 8, 2024
@mp911de mp911de added this to the 7.x milestone Apr 8, 2024
@tishun tishun modified the milestones: 7.x, 7.0.0.RELEASE, 6.4.0.RELEASE Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vladvildanov vladvildanov vladvildanov approved these changes

@tishun tishun tishun approved these changes

@mp911de mp911de mp911de approved these changes

Assignees
No one assigned
Labels
type: feature A new feature
Projects
None yet
Milestone
6.4.0.RELEASE
Development

Successfully merging this pull request may close these issues.

Add support for SSUBSCRIBE
4 participants
@atakavci @tishun @mp911de @vladvildanov