Skip to content

Fixed the empty string issue when decoding MBULK_REPLY #246

Closed
wants to merge 8 commits into from

2 participants

@zhanggl
zhanggl commented Feb 27, 2014

Motivations:

Similar to #245, MBULK reply containing an empty string can not be properly decoded.

Modifications:

Reply.scala and related tests

zhanggl added some commits Feb 24, 2014
@zhanggl zhanggl fixed bug(#244): "" -> "$0\r\n\r\n" instead of "$-1\r\n" according to h…
…ttp://redis.io/topics/protocol

And, the error reply of incorrect data type switched from 'ERR' to 'WRONGTYPE' in test
ca3b856
@zhanggl zhanggl fixed indentation a56de45
@zhanggl zhanggl removed an extra blank line 25ff0df
@zhanggl zhanggl Removed the redundant case 0 branch
Removed the case 0 pattern matching branch for empty string which is redundant in function decodeBulkReply
9e981da
@zhanggl zhanggl Fixed a test to temporarily support redis server 2.8
In Redis 2.8, Many errors are now prefixed by a more specific error code instead of the generic -ERR, for example -WRONGTYPE, -NOAUTH, ...
e59f5cf
@mosesn

this will be a little faster if we use endsWith instead of contains

@mosesn
mosesn commented Feb 27, 2014

Although it's OK to do these two pull requests concurrently, it will be confusing if we try to apply the same commits again. We shouldn't have to repeat commits. Could you make a new pull request which doesn't include the repeat commits? One way of doing this would be to make a patch that includes only the changes from ad2d291 and applying that to the current master branch and making that the branch for 246.

Could you also elaborate on the results of this change and what precisely the modifications entail?

@zhanggl
zhanggl commented Feb 27, 2014

ad2d291 does depend on changes in ca3b856 to pass all the tests:
1. BulkReply needs to be changed to allow having empty message be encoded and decoded
2. The WRONGTYPE error message thing

I will say that let's just wait for #245 to be finally merged in master.

@mosesn
mosesn commented Feb 27, 2014

Got it. Good idea.

@zhanggl
zhanggl commented Mar 8, 2014

Out-of-date. see #251

@zhanggl zhanggl closed this Mar 8, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.