Skip to content

DATAREDIS-293 - JedisConnection, allow bulk addition with same score. #63

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

Closed
wants to merge 2 commits into from

Conversation

christophstrobl
Copy link
Member

Merged and polished pull request #62.
Added jedis version check to retain behavior for older versions.

christophstrobl added a commit that referenced this pull request Apr 7, 2014
Added version check for jedis library as the operation had been introduced in 2.4.

Original Pull Request: #63
@RyanAD
Copy link
Contributor

RyanAD commented Apr 7, 2014

I think the code had already been changed in an incompatible way before my pull request. If you look at the method signature for zadd in 2.2 vs 2.4 the map key and value were swapped:

2.2: https://github.com/xetorthio/jedis/blob/0defe548c941ac55ac5293b8945635bb97cf3d79/src/main/java/redis/clients/jedis/Jedis.java#L1409

2.4: https://github.com/xetorthio/jedis/blob/70fa35f3ba13b90d74cf98095e7525d10e1cf80a/src/main/java/redis/clients/jedis/Jedis.java#L1424

JedisConnection had already been changed to use the new Map layout without doing a version check so I wasn't sure if 2.2 was still supported.

So even with this check, adding multiple elements with different scores is actually broke because the score and element are inverted in the map. If the goal is to keep supporting Jedis 2.2 then we may need two versions of zAddArgs method.

christophstrobl and others added 2 commits April 11, 2014 10:36
Added version check for jedis library as the operation had been introduced in 2.4.

Original Pull Request: #63
thomasdarimont pushed a commit that referenced this pull request Apr 11, 2014
Added version check for jedis library as the operation had been introduced in 2.4.

Original Pull Request: #63
@thomasdarimont
Copy link

Merged into master 6d7c5ce

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.

3 participants