[Zookeeper-developer] [ zookeeper-Bugs-1948141 ] KeeperException should be sub-classed

[SourceForge.net <no...@so...>]

Bugs item #1948141, was opened at 2008-04-21 11:02
Message generated for change (Comment added) made by breed
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1948141&group_id=209147

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: 3.0.0
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Ted Dunning (tedunning)
Assigned to: Benjamin Reed (breed)
Summary: KeeperException should be sub-classed

Initial Comment:

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.

----------------------------------------------------------------------

>Comment By: Benjamin Reed (breed)
Date: 2008-05-12 16:03

Message:
Logged In: YES 
user_id=154690
Originator: NO

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

----------------------------------------------------------------------

Comment By: Patrick Hunt (phunt)
Date: 2008-05-05 16:00

Message:
Logged In: YES 
user_id=12853
Originator: NO

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


----------------------------------------------------------------------

Comment By: Benjamin Reed (breed)
Date: 2008-04-22 13:51

Message:
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.

----------------------------------------------------------------------

Comment By: Ted Dunning (tedunning)
Date: 2008-04-22 11:46

Message:
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

----------------------------------------------------------------------

Comment By: Ted Dunning (tedunning)
Date: 2008-04-22 11:45

Message:
Logged In: YES 
user_id=972206
Originator: YES


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

----------------------------------------------------------------------

Comment By: Benjamin Reed (breed)
Date: 2008-04-22 08:30

Message:
Logged In: YES 
user_id=154690
Originator: NO

+1 Excellent suggestion! A patch would be great.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1948141&group_id=209147