Skip to content

Add RM_PublishMessageShard#10543

Merged
oranagra merged 7 commits intoredis:unstablefrom
guybe7:api_spublish
Apr 17, 2022
Merged

Add RM_PublishMessageShard#10543
oranagra merged 7 commits intoredis:unstablefrom
guybe7:api_spublish

Conversation

@guybe7
Copy link
Copy Markdown
Collaborator

@guybe7 guybe7 commented Apr 6, 2022

since PUBLISH and SPUBLISH use different dictionaries for channels and clients, and we already have an API for PUBLISH, it only makes sense to have one for SPUBLISH

Add test coverage and unifying some test infrastructure.

Comment thread src/pubsub.c Outdated
@oranagra
Copy link
Copy Markdown
Member

oranagra commented Apr 6, 2022

@redis/core-team please approve the new API.
@guybe7 please write something useful (what, why) in the top comment.

@oranagra oranagra added state:major-decision Requires core team consensus approval-needed Waiting for core team approval to be merged labels Apr 6, 2022
Copy link
Copy Markdown
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly LGTM

Comment thread src/cluster.c Outdated
@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Apr 7, 2022
@oranagra
Copy link
Copy Markdown
Member

i see we have 0 coverage of these in the tests, let's add something trivial, just to get this code running.

Other:
Cleanup pubsubshard.tcl (not sure why the file had a copy of the code in util.tcl in the first place...)
Comment thread tests/unit/pubsubshard.tcl
@oranagra oranagra merged commit f49ff15 into redis:unstable Apr 17, 2022
@oranagra oranagra mentioned this pull request Apr 27, 2022
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
since PUBLISH and SPUBLISH use different dictionaries for channels and clients,
and we already have an API for PUBLISH, it only makes sense to have one for SPUBLISH

Add test coverage and unifying some test infrastructure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants