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

redis-integer-reply #114

Merged
merged 1 commit into from May 23, 2013
Merged

Conversation

charsyam
Copy link
Contributor

fixing parsing redis response to accept integer-reply

@charsyam
Copy link
Contributor Author

@manjuraj please, review this code :)

@manjuraj
Copy link
Collaborator

We need to fix this generically. Based on redis.io/topics/protocol "every element of a Multi Bulk Reply can be of any kind, including a nested Multi Bulk Reply". Currently the response parser assumes that every element of the multi bulk reply is a bulk reply and we have to change that

@charsyam
Copy link
Contributor Author

@manjuraj Yes. you're right. I just think that Multi Bulk reply consists of "String" + "Integer Reply". Thank you.
I will think about it right :)

@manjuraj
Copy link
Collaborator

every element of the multi-bulk reply can be integer / bulk / nested multi-bulk reply

@manjuraj
Copy link
Collaborator

I think we can just solve it for integer and bulk reply for now; nested multi-bulk reply might be hairy to implement

@charsyam
Copy link
Contributor Author

@manjuraj I think nested multi-bulk reply is impossible in redis. and multi-bulk just reply can be integer/bulk.
So. I might think this patch can parse redis response right because nested multi-bulk is impossible. :)

@manjuraj
Copy link
Collaborator

we need to fix '$-1' cases as bulk element, I believe

@charsyam
Copy link
Contributor Author

@manjuraj Yes, and we need to fix '*-1' cases also. :)

@charsyam
Copy link
Contributor Author

@manjuraj in previous code, you already handle "$-1" case :)

redis-integer-reply
@charsyam
Copy link
Contributor Author

@manjuraj I tested following cases :) and it passed all cases.
Please review this patch :) Thank you

get abc
$-1

mget abc ade ged
*3
$-1
$-1
$-1

eval "return {10,10}" 1 TEMP
*2
:10
:10

blpop key 1 <--- for test, I changed can support this :) only for using one connection.
*-1

@manjuraj
Copy link
Collaborator

good job @charsyam

manjuraj added a commit that referenced this pull request May 23, 2013
support integer reply in multi-bulk reply
@manjuraj manjuraj merged commit 0469c3c into twitter:master May 23, 2013
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