coolexp / redis

Automatically exported from code.google.com/p/redis
0 stars 0 forks source link

Error strings should all start with -ERR #8

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
In the new version of the protocol when there's a GET-like operation on any 
type the format of the 
response is:

-<ERRNO>
<OPERATION> against something...

To simplify parsing of the response it should probably be:

-<ERRNO>
-ERR: <OPERATION> against something

Original issue reported on code.google.com by dialt...@gmail.com on 28 Feb 2009 at 5:02

GoogleCodeExporter commented 8 years ago
dialtone: before to reply, note that is not -ERRNO but
-(size-of-the-error-following-in-bytes).

Original comment by anti...@gmail.com on 28 Feb 2009 at 5:15

GoogleCodeExporter commented 8 years ago
p.s. I mean: I wrote this info to you before to write a reply to your message 
because
I'm not sure this still applies given that the negative number is the number of 
bytes
(once you ABS it).

Original comment by anti...@gmail.com on 28 Feb 2009 at 5:16

GoogleCodeExporter commented 8 years ago
Oh, uhmm... Why using the -{size}? do you plan to have \r\n in the error 
description? If not it's redundant 
because the error will be in its own line anyway so client libraries could 
safely ignore the length of the error 
because the description will be on a line on its own. Otherwise, so you want to 
have \r\n in error descriptions, I 
need to change error handling in these cases.

Original comment by dialt...@gmail.com on 28 Feb 2009 at 5:24

GoogleCodeExporter commented 8 years ago
dialtone: the idea is that you are in bulk mode... so the error message may be 
read
with the same codepath, in pseudocode it should be as simple as:

len = ...
if (len < 0) error = true
buffer = .. bulk read code ..
if error
  exception(buffer)
else
  return buffer
end

Original comment by anti...@gmail.com on 2 Mar 2009 at 9:30

GoogleCodeExporter commented 8 years ago
But that's not the point. It's actually easier to not have to switch to raw 
mode. As I said, unless the error 
description has a \r\n it's actually the same and would require at least a 
syscall less. For me there's little 
difference, this code hits the same state machine anyway here: 
http://bitbucket.org/adroll/erldis/src/tip/src/client.erl#cl-198 I deal with 
the error by returning the atom 
'error'  from the line parser and then interpreting the next line as an error 
description. If I needed to switch to 
raw mode I'd need to add more logic to that same branch of the case, I'd need 
to switch to raw mode, read the 
bytes+2 and switch back to line mode. It's indeed not a big deal, 2-3 lines 
more. It's more about the future 
format of the error description, if you tell me that it will never contain a 
\r\n then I won't change it, otherwise 
I'll change it to use raw mode.

Original comment by dialt...@gmail.com on 2 Mar 2009 at 9:37

GoogleCodeExporter commented 8 years ago
dialtone: ok got the point: since there will never be an error with \r\n inside 
I
guess it can be a good move to just add -ERR before every bulk reply error, 
this way
the protocol will be compatible with both the ways of reading it.

Original comment by anti...@gmail.com on 2 Mar 2009 at 9:42

GoogleCodeExporter commented 8 years ago
This will also fix Issue 11.

Original comment by ludovico...@siassb.eu on 2 Mar 2009 at 9:44

GoogleCodeExporter commented 8 years ago
Ludo: issue 11 does not exist! ;) Try it by telnet.

Original comment by anti...@gmail.com on 2 Mar 2009 at 9:54

GoogleCodeExporter commented 8 years ago
Done, now all the error messages, bulk messages or reply code messages, are 
prefixed
with -ERR. This does not break old code and makes all the error messages 
formatted
the same. I don't think there was a problem before but now is fine too.

Original comment by anti...@gmail.com on 2 Mar 2009 at 10:03

GoogleCodeExporter commented 8 years ago
You really like the redundant string length in this case :). Anyway I updated 
my client library, thanks.

Original comment by dialt...@gmail.com on 2 Mar 2009 at 10:25