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 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 requested a review from gkorland Oct 16, 2019
@sazzad16 sazzad16 self-assigned this Oct 16, 2019
@sazzad16

This comment has been minimized.

Copy link
Collaborator Author

sazzad16 commented Oct 16, 2019

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

Copy link
Contributor

DvirDukhan left a comment

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) {

This comment has been minimized.

Copy link
@gkorland

gkorland Nov 25, 2019

Collaborator

@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

This comment has been minimized.

Copy link
Collaborator

gkorland commented Nov 25, 2019

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

@sazzad16

This comment has been minimized.

Copy link
Collaborator Author

sazzad16 commented Nov 25, 2019

@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 xetorthio:master Dec 9, 2019
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@sazzad16 sazzad16 removed the READY TO MERGE label Dec 9, 2019
@sazzad16 sazzad16 deleted the sazzad16:sendCommand branch Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.