#14 KeeperException should be sub-classed

3.0.0
closed-fixed
None
5
2008-05-12
2008-04-21
Ted Dunning
No

The current practice of setting an error code in KeeperException leads to error prone code that looks like this when only one kind of exception can be handled:

void createOrUpdate(path, String data) {
try {
create(path, data)
} catch (com.yahoo.zookeeper.KeeperException e) {
if (e.getCode() == com.yahoo.zookeeper.KeeperException.Code.NodeExists) {
update(path, data, -1)
} else {
throw e;
}
}
}

(ignore the lack of semi's... this is groovy)

It would be much better to be able to write:

void createOrUpdate(path, String data) {
try {
create(path, data)
} catch (com.yahoo.zookeeper.NodeExistsException e) {
update(path, data, -1)
}
}

Where NodeExistsException extends KeeperException. The getCode method can be implemented as before so that old code will not notice the change.

This change makes the exception handling much more Java friendly and less like C.

I can create a patch if desired.

Discussion

  • Benjamin Reed

    Benjamin Reed - 2008-04-22

    Logged In: YES
    user_id=154690
    Originator: NO

    +1 Excellent suggestion! A patch would be great.

     
  • Ted Dunning

    Ted Dunning - 2008-04-22
     
  • Ted Dunning

    Ted Dunning - 2008-04-22

    Logged In: YES
    user_id=972206
    Originator: YES

    Here is a patch. See the patch comments for details.
    File Added: 1948141.patch

     
  • Ted Dunning

    Ted Dunning - 2008-04-22

    Logged In: YES
    user_id=972206
    Originator: YES

    One thing that I should mention is that this patch fixes a minor bug in KeeperException where not all codes had an associated message. I added a message for NotEmpty

     
  • Benjamin Reed

    Benjamin Reed - 2008-04-22

    Logged In: YES
    user_id=154690
    Originator: NO

    Fantastic work Ted! Do you mind submitting the patch to the patch tracker? That way we can tie it to a release. We are going to do one more release and then we will be putting in some API breaking patches. They will all be slight breaks, like the one here (changing a package, using an enum instead of int, etc...).

    My one other nit pick would be that in create() when you throw IllegalArgumentException() you put the error code number in the message.

     
  • Patrick Hunt

    Patrick Hunt - 2008-05-05
    • assigned_to: nobody --> breed
     
  • Patrick Hunt

    Patrick Hunt - 2008-05-05

    Logged In: YES
    user_id=12853
    Originator: NO

    Ben is working this out on patch 1949196 with Ted, assigning to Ben.

     
  • Benjamin Reed

    Benjamin Reed - 2008-05-12
    • milestone: --> 3.0.0
     
  • Benjamin Reed

    Benjamin Reed - 2008-05-12

    Logged In: YES
    user_id=154690
    Originator: NO

    Fixed in patch: [ 1949196 ] KeeperException should be sub-typed

     
  • Benjamin Reed

    Benjamin Reed - 2008-05-12
    • status: open --> closed-fixed
     

Log in to post a comment.