-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
PipelineBase should not contain multi key operations #2079
Conversation
PipelineBase should not 'look like' it contains multi key operations
@DvirDukhan PING! as you were the author of related PRs. |
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.
LGTM
public Response<Object> sendCommand(ProtocolCommand cmd, String... args){ | ||
String key = args.length > 0 ? args[0] : cmd.toString(); | ||
getClient(key).sendCommand(cmd, args); | ||
public Response<Object> sendCommand(final String sampleKey, final ProtocolCommand cmd, final String... args) { |
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.
@sazzad16 I think we should just deprecate these methods, once you have them in the MultiKeyPipelineBase you don't need these.
I don't think we need those for ShardedJedisPipeline or just move them there with the sample keys.
@sazzad16 I think we should also add these methods to the interfaces MultiKeyBinaryRedisPipeline and MultiKeyCommandsPipeline |
@gkorland MultiKeyBinaryRedisPipeline and MultiKeyCommandsPipeline contains actual Redis commands. Because of this conflict, we already have an open issue #2029 related to this. |
PipelineBase should not look like it contains multi key operations.
This is modification of #2007 and #2025