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.

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

Merged
merged 1 commit into from Dec 9, 2019

Conversation

sazzad16
Copy link
Collaborator

@sazzad16 sazzad16 commented Oct 16, 2019

PipelineBase should not look like it contains multi key operations.

This is modification of #2007 and #2025

PipelineBase should not 'look like' it contains multi key operations
@sazzad16 sazzad16 added this to the 3.2.0 milestone Oct 16, 2019
@sazzad16 sazzad16 self-assigned this Oct 16, 2019
@sazzad16
Copy link
Collaborator Author

@DvirDukhan PING! as you were the author of related PRs.

Copy link
Contributor

@DvirDukhan DvirDukhan left a 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) {
Copy link
Contributor

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.

@gkorland
Copy link
Contributor

@sazzad16 I think we should also add these methods to the interfaces MultiKeyBinaryRedisPipeline and MultiKeyCommandsPipeline

@sazzad16
Copy link
Collaborator Author

@gkorland MultiKeyBinaryRedisPipeline and MultiKeyCommandsPipeline contains actual Redis commands. Because of this conflict, we already have an open issue #2029 related to this.

@sazzad16 sazzad16 merged commit 7f6bfc7 into redis:master Dec 9, 2019
@sazzad16 sazzad16 deleted the sendCommand branch March 11, 2020 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants