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

[Compatibility] Added PUBSUB CHANNELS, NUMPAT and NUMSUB commands #719

Merged
merged 18 commits into from
Oct 28, 2024
Merged

[Compatibility] Added PUBSUB CHANNELS, NUMPAT and NUMSUB commands #719

merged 18 commits into from
Oct 28, 2024

Conversation

Vijay-Nirmal
Copy link
Contributor

@Vijay-Nirmal Vijay-Nirmal commented Oct 13, 2024 •
edited
Loading

Adding PUBSUB CHANNELS, NUMPAT and NUMSUB command to garnet

  • Add PUBSUB CHANNELS, NUMPAT and NUMSUB commands
  • Add Integration Test cases, ACL Test and Slot Verification Test (There is no ClusterSlotVerificationTests for current publish command)
  • Add documentation

Fixes: #425

@TalZaccai TalZaccai self-requested a review October 15, 2024 18:35
@Vijay-Nirmal
Copy link
Contributor Author

@badrishc @TalZaccai RespCommand can no longer be of type byte, we have more that 255 commands. Will you guys be working on changing the type? We can't merge this PR till then. Also, I can't work adding more commands till we fix this.

@TalZaccai
Copy link
Contributor

@badrishc @TalZaccai RespCommand can no longer be of type byte, we have more that 255 commands. Will you guys be working on changing the type? We can't merge this PR till then. Also, I can't work adding more commands till we fix this.

Thanks for the heads up @Vijay-Nirmal! #736 should address this.

TalZaccai
TalZaccai requested changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@TalZaccai TalZaccai left a comment

Choose a reason for hiding this comment

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

Added some initial comments...

libs/server/PubSub/SubscribeBroker.cs Outdated Show resolved Hide resolved
libs/server/PubSub/SubscribeBroker.cs Outdated Show resolved Hide resolved
libs/server/PubSub/SubscribeBroker.cs Show resolved Hide resolved
libs/server/PubSub/SubscribeBroker.cs Outdated Show resolved Hide resolved
libs/server/PubSub/SubscribeBroker.cs Outdated Show resolved Hide resolved
libs/server/Resp/PubSubCommands.cs Outdated Show resolved Hide resolved
test/Garnet.test/RespPubSubTests.cs Outdated Show resolved Hide resolved
Vijay-Nirmal

This comment was marked as duplicate.

Sign in to view

@Vijay-Nirmal
Copy link
Contributor Author

Fixed the current review comments with one open question

@Vijay-Nirmal
Copy link
Contributor Author

Fixed all review commands

@TalZaccai TalZaccai merged commit 6d489c6 into microsoft:main Oct 28, 2024
15 checks passed
@Vijay-Nirmal Vijay-Nirmal deleted the new/pubsub-channels-numpat-numsub branch October 28, 2024 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TalZaccai TalZaccai TalZaccai approved these changes

@Meir017 Meir017 Awaiting requested review from Meir017

@badrishc badrishc Awaiting requested review from badrishc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

API - PUBSUB CHANNELS
4 participants
@Vijay-Nirmal @TalZaccai @Meir017 @badrishc