As I was implementing read-only mode in the Python client based on the Java client patch, I noticed a rather odd naming for the error you get if you send a modification command to a read-only server...NotReadOnlyException.

Why the sudden change in error context?

For reference, here's some of the other errors that Zookeeper may return when making an API call:
NoNode
NoAuth
BadVersion
NoChildrenForEphemerals
NodeExists
NotEmpty

So the explanation for these errors are consistent, "your API call cannot be completed because of this state on the server". Personally, I'm a huge fan of consistency in an API, so these are all great. But then with NotReadOnly, we have an error that is not referring to the state of the server (that it *is* ReadOnly), but one that refers to the semantics of the API call itself. Given all the other errors, I was really expecting the server to throw a ReadOnly error indicating your call cannot be completed due to that state on the server (like the others).

Was there a reason for the context switch in error naming? I understand given its been merged in for almost 2 years now that there's unlikely to be any switch to make it consistent in context with the other errors, but it might be nice for future feature additions to try and document or enforce better consistency in the API.

Cheers,
Ben

On 7 September 2012 10:07, Ben Bangert <[EMAIL PROTECTED]> wrote:

> As I was implementing read-only mode in the Python client based on the
> Java client patch, I noticed a rather odd naming for the error you get if
> you send a modification command to a read-only
> server...NotReadOnlyException.
>
> Why the sudden change in error context?
>
> For reference, here's some of the other errors that Zookeeper may return
> when making an API call:
> NoNode
> NoAuth
> BadVersion
> NoChildrenForEphemerals
> NodeExists
> NotEmpty
>
> So the explanation for these errors are consistent, "your API call cannot
> be completed because of this state on the server". Personally, I'm a huge
> fan of consistency in an API, so these are all great. But then with
> NotReadOnly, we have an error that is not referring to the state of the
> server (that it *is* ReadOnly), but one that refers to the semantics of the
> API call itself. Given all the other errors, I was really expecting the
> server to throw a ReadOnly error indicating your call cannot be completed
> due to that state on the server (like the others).
>
> Was there a reason for the context switch in error naming? I understand
> given its been merged in for almost 2 years now that there's unlikely to be
> any switch to make it consistent in context with the other errors, but it
> might be nice for future feature additions to try and document or enforce
> better consistency in the API.
>
>
What do you think a good exception name would be? (Or do you think an
exception is the wrong error path here?)

My view is that the exception is badly named, and should indicate the
actual error, like NodeNotWriteable or similar. It is, unfortunately, hard
to make these changes in minor releases though, although I think we could
consider it for 3.5.

cheers,
Henry
> Cheers,
> Ben
--
Henry Robinson
Software Engineer
Cloudera
415-994-6679

On Sep 11, 2012, at 10:28 AM, Henry Robinson <[EMAIL PROTECTED]> wrote:

> What do you think a good exception name would be? (Or do you think an
> exception is the wrong error path here?)

Given that the other errors all reflect what specifically is responsible for the error given the servers state, I was expecting an error like:
ServerIsReadOnly

Which clearly indicates the call failed because the server is read-only. The concept of "read-only commands" is kind of strange, which is what the current NotReadOnly exception refers to (the call itself is not a read-only call). ServerIsReadOnly fits into the current scheme of:
Command X failed due to condition Y on the server.

> My view is that the exception is badly named, and should indicate the
> actual error, like NodeNotWriteable or similar. It is, unfortunately, hard
> to make these changes in minor releases though, although I think we could
> consider it for 3.5.

Makes sense.

Cheers,
Ben