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

change exists command to variodic #1054

Merged
merged 5 commits into from Aug 7, 2015
Merged

Conversation

charsyam
Copy link

today, redis exists command is changed to variodic.

import redis.clients.jedis.Protocol.Command;
import redis.clients.jedis.Protocol.Keyword;
import redis.clients.util.SafeEncoder;
import static redis.clients.jedis.Protocol.Command.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

@charsyam
Mixed define of * and specific cases in import static Command, which would be better to fix it.
Using one way would be fine, so you can just remove specific cases import statements.

@HeartSaVioR
Copy link
Contributor

@charsyam
Looks fine to me, I left some comments which guides you to respect semver, and some other things.

Btw, your changeset breaks indentation, which can be fixed by running make format.
Please refer https://github.com/xetorthio/jedis/blob/master/CONTRIBUTING.md#code-convention for more details.

Thanks for the great work!

@HeartSaVioR
Copy link
Contributor

@charsyam
I've found some more things to do.
Please make some effort to

  1. BinaryJedisCommands
    a. add Long exists(byte[][] key);
    b. mark @deprecated to Boolean exists(byte[] key);
  2. BinaryClient
    a. apply my comment (maybe missing)

Other things are fine. Whenever you're completing your work, I'll label as WAIT FOR MORE REVIEWS.
Thanks!

@HeartSaVioR
Copy link
Contributor

Talking with @charsyam , I completely forgot to tell while adding new commands to Jedis.
Since operation we'd like to add it multi keys operation, we should start adding method signature to interfaces below,

  • multi key, binary: MultiKeyBinaryCommands
  • multi key, binary, cluster: MultiKeyBinaryJedisClusterCommands
  • multi key, binary, pipeline: MultiKeyBinaryRedisPipeline
  • multi key, normal: MultiKeyCommands
  • multi key, normal, cluster: MultiKeyJedisClusterCommands
  • multi key, normal, pipeline: MultiKeyCommandsPipeline

It should be safe with ShardedJedis* cause ShardedJedis only treats singular key operations.

@charsyam Sorry about the confusing. It must be documented to CONTRIBUTING.md soon.

@HeartSaVioR
Copy link
Contributor

@charsyam
Looks great to me.
Last one thing I'd like to ask a favor is adding @deprecated annotation to JedisCluster.exists(final String key).

@xetorthio @marcosnils
As you know, there should be two different PRs which one deprecates methods at 2.7 / 2.8 version lines, and next one removes method at 3.0.0.
This PR doesn't respect it, but I think I can manage it. I'd like to adding some modifications during merging step.

@HeartSaVioR
Copy link
Contributor

@xetorthio @marcosnils
We decided to remove ShardedJedis at 3.0.0, right?
Deprecating exist() with singular key is based on decision. (If we don't remove ShardedJedis, we should maintain two version of methods, singular key and multiple key.)

Please review and comment. Thanks!

@HeartSaVioR
Copy link
Contributor

FYI: exists with vargs is included to Redis 3.0.3.

@marcosnils
Copy link
Contributor

@HeartSaVioR haven't had time to review, sorry. I'll take a look tomorrow.

@charsyam
Copy link
Author

Redis 3.0.3 is out. and It contains Variadic Exists.

@HeartSaVioR
Copy link
Contributor

Since it didn't reviewed for long time, I think it's better to merge than waiting more.

@HeartSaVioR HeartSaVioR merged commit b540775 into redis:master Aug 7, 2015
@HeartSaVioR
Copy link
Contributor

@charsyam Thanks for amazing work! I merged into master and 2.8 respectively.

@charsyam
Copy link
Author

charsyam commented Aug 7, 2015

@HeartSaVioR Thanks. :)

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