New ZADD Options

[jpe42]

Redis recently added new options to the ZADD command as of 3.0.2. Is anyone already planning on adding these? I took a look at it, but it gets a bit hairy because the INCR option changes the return data type... Is it desired to add a new method interface for every combination of options?

[HeartSaVioR]

@jamespedwards42
Sorry I couldn't follow up Redis releases some months so I didn't know that.
I'll try to implement it later, but contribution is really appreciated!

I've read ZADD page from Redis doc. now, and feel that we can't add INCR option with current way cause return type should be changed to Double.

ZADD with INCR option is same to ZINCRBY, so I'd like to not supporting it.
(EDITED: It is not completely same cause ZADD with INCR option also supports NX / XX options.)
@xetorthio @marcosnils What do you think?

Answering your last question, sure it is NOT desired to add a new method interface for every combination of options.
We try to create only one method which covers every combination of options, by introducing *Param class.
You can refer #878 to see how we handle combination of options.

[marcosnils]

@HeartSaVioR I guess we can include it after #878 has been merged. In the mean time I guess ZINCRBY pretty much is the same thing right?.

@jamespedwards42 what do you think?. Is there a reason you need to explicitly use ZADD with INCR option instead of ZINCRBY?

[HeartSaVioR]

@marcosnils ZADD with INCR option can be used with NX / XX option, which ZINCRBY is not supported.

[HeartSaVioR]

@xetorthio @marcosnils @jamespedwards42
I think adding NX / XX option to ZINCRBY makes sense, which doesn't change return type.
Current way doesn't make sense, and I think it should be changed.

I posted this issue and alternative way to Redis user mailing groups.
https://groups.google.com/d/msg/redis-db/A-a_OYZO8Q0/xym93jJI0qIJ

[HeartSaVioR]

@marcosnils
I pointed #878 as a reference. We can introduce ZAddParam (or any beter naming) to do the same thing without #878.

[marcosnils]

@HeartSaVioR I guess it makes sense. Let's wait to see if we get any feedback in the redis groups and decide what to do about this.

[jpe42]

@HeartSaVioR @marcosnils
I agree that it is weird that they didn't just add the options to ZINCRBY, however I'm not sure what the history is of them reverting features once they have been released. I think it would be reasonable to add one additional method explicitly for using the INCR option, e.g., zaddincrby(). Note that it doesn't need to be added for the call that updates multiple members.

[HeartSaVioR]

@jamespedwards42 @marcosnils
I don't think we need to add one additional method if Redis adds NX / XX options to ZINCRBY whether antirez removes INCR option from ZADD or not.

I know that reverting anything which is already released could be really pain for everyone.
But Jedis overcomes that pain some moments ago, and it would be a great lesson for Redis, too.

[HeartSaVioR]

Posted this issue to Github issue, too.
redis/redis#2697

[HeartSaVioR]

Hmm... Now I don't expect antirez changes his mind.
I think I can stick with badboy's suggestion - change zincrby()'s implemention to use zadd internally - since we can change implementation of zincrby() later without modifying its signature.

@jamespedwards42 @marcosnils What do you think?

[jpe42]

I think it makes perfect sense to put it under zincrby().

...Just mentioning because I didn't notice it in the rest of the conversation, the CH option would also be applicable to use in conjunction with the INCR option.

[HeartSaVioR]

@jamespedwards42
No, if you specify CH option with INCR option, it is ignored.

127.0.0.1:6379> zadd 10 CH INCR 3 "b"
"3"
127.0.0.1:6379> zadd 10 INCR 3 "b"
"6"
127.0.0.1:6379> zadd 10 CH INCR 3 "b"
"9"

[jpe42]

Ah yea, that makes sense anyways considering you can only use INCR with one score-element pair and therefore already know it will change.

[HeartSaVioR]

@jamespedwards42 Yes, right.

[HeartSaVioR]

@jamespedwards42 @marcosnils
I realized that we can't cover zincrby() with ZADD + INCR option cause it doesn't work with Redis 3.0.1 and lower.
Seems like we can avoid this issue to use ZADD only whenever users call zincrby() with NX / XX option.
(Sure it doesn't work with Redis 3.0.1 and lower, so users should keep in mind.)

[marcosnils]

@HeartSaVioR let's do that, let's only call ZADD + INCR when NX / XX is set in the zincrby method.

[HeartSaVioR]

@marcosnils OK! I'll take care of it. Actually I'm working on it. :)

[marcosnils]

@HeartSaVioR it'd be great if you could add to the javadoc that the command is supported from redis 3.0.1 onwards

[HeartSaVioR]

@marcosnils OK. I'll add it.

[HeartSaVioR]

@jamespedwards42 @marcosnils
I'm done. Please review #1083 and comment. Thanks in advance!