Added support for sharded pub/sub consumer #1303
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
leibale
reviewed
May 30, 2023
Comment on lines
+406
to
+408
| // SUNSUBSCRIBE command could be called without arguments, so it doesn't matter on each node it will be called. | ||
| if (empty($arguments)) { | ||
| return 'fake'; |
There was a problem hiding this comment.
"SUNSUBSCRIBE command could be called without arguments, so it doesn't matter on each which (?) node it will be called."
why return 'fake'; and not return null;?
There was a problem hiding this comment.
also, why this function is here and not in the SUNSUBSCRIBE class?
There was a problem hiding this comment.
- In Predis we're using pre-calculation (same algorithm that redis do) of slots to determine node that should accept command, it works well for commands that runs against specific key/keys, but if there's no key you still need to pre-calculate node so it's just random string returns to allow this action.
- It's just because of bad design, I'm totally agree it should be a part of command object and do not violate SRP, but fixing it is beyond the scope of RESP3
There was a problem hiding this comment.
- but that means that the node that corresponds to
'fake'(slot 14787) will be used more than other nodes..
There was a problem hiding this comment.
For SUNSUBSCRIBE command only which is okay??, since it's not something you're often sends to server, but I can use randomly generated string here instead
Closed
chayim
approved these changes
Jul 19, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SSUBSCRIBE,SUNSUBSCRIBE,SPUBLISHcommands.Consumerclass, to allow run over cluster connection in sharded pub/sub context.RedisClusterconnection to be able to loop over sockets and read incoming messages from Redis server.DispatcherLoopto run pub/sub commands depends on subscription context.Closes #971
Closes #972
Closes #973