[Zookeeper-developer] [ zookeeper-Patches-1949196 ] KeeperException should be sub-typed

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 422-6466

Patches item #1949196, was opened at 2008-04-22 15:07
Message generated for change (Comment added) made by breed
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1949196&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: Closed
>Resolution: Fixed
Priority: 5
Private: No
Submitted By: Ted Dunning (tedunning)
Assigned to: Benjamin Reed (breed)
Summary: KeeperException should be sub-typed

Initial Comment:
Note that one of the tests doesn't seem to run for me.  This does not look related to this patch.  I am having some disk space issues just now that are preventing me from running this test again.

This patch turned out a bit bigger than expected because I went back into all of the code and made the construction of KeeperExceptions as specific as possible.  At the same time, I trimmed down as many of the throws declarations as I could.  This is the second patch in this series.  It changes an error message in KeeperException.create to be more clear.

Specifically:

* The generic constructors for KeeperException were hidden to prevent direct construction.
* All direct constructors of KeeperException were hidden in the zookeeper.exceptions package
* A factory method was created for constructing sub-classed KeeperExceptions based on error code
* All constructions of KeeperException were examined and converted to direct constructions of the specific sub-types or to a call to the factory method
* A few instances of catch(KeeperException) were made more specific to avoid the immediately following if-statement
* Some cases of redundant catches were removed
* Some javadoc was correct vis a vis Exceptions thrown
* Some unused imports were removed from TestClient

Some gratuitous changes in the order of imports were introduced as an artifact of the semi-automated 
method used to do these changes.

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

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

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

Committed revision 165.

Thanx Ted!

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

Comment By: Ted Dunning (tedunning)
Date: 2008-05-12 15:41

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

Here is a slight fix.
File Added: 1948141-d.patch

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

Comment By: Benjamin Reed (breed)
Date: 2008-05-12 15:28

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

I think you might be using an old trunk. I'm getting patch failures.

There are also a couple of changes you are still reverting in ClientCnxn:
you shouldn't be reverting the final decls. In ZooKeeper: you are reverting
the watcher fix -- the watcher needs to be set before the ClientCnxn is
constructed.

As soon as I get a working patch I'll apply it. (I want this one to be the
next one to go in.) Thanx Ted!

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

Comment By: Ted Dunning (tedunning)
Date: 2008-05-12 14:52

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

Here is a revised patch that goes against trunk.

It should be applied soon to avoid another set of conflicts.
File Added: 1948141-d.patch

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

Comment By: Patrick Hunt (phunt)
Date: 2008-05-12 14:09

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

sf has been flakey lately. Had a bunch of issues last week trying to
update tracker items.

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

Comment By: Ted Dunning (tedunning)
Date: 2008-05-12 14:06

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

I definitely missed some updates.  I have dealt with the conflicts, but I
can't seem to update from SVN right now.  It worked 10 minutes ago, but not
now.

Time for tea.

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

Comment By: Ted Dunning (tedunning)
Date: 2008-05-12 13:39

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

I will take a look.  

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

Comment By: Benjamin Reed (breed)
Date: 2008-05-01 15:09

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

It looks like this code reverts a change in ClientCnxn. Ted, can you see
if you missed an update?

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

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

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

+1 this looks good to me.

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

Comment By: Benjamin Reed (breed)
Date: 2008-05-01 14:52

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

+1 I like it!
We need to hold this for the start of the 3.0.0 release since it is a
slight API break, even though I doubt anyone would notice.

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

Comment By: Ted Dunning (tedunning)
Date: 2008-05-01 14:48

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

Yes.

I was going to make them inner.

Here is another replacement patch.

File Added: 1948141-c.patch

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

Comment By: Ted Dunning (tedunning)
Date: 2008-05-01 14:42

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

I am sure it is just you that caused me to create a half-patch.  :-)

Here is a corrected one.  I deleted the broken one.
File Added: 1948141-c.patch

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

Comment By: Benjamin Reed (breed)
Date: 2008-05-01 14:42

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

Yeah I think he for got them. Weren't you going to nest them in
KeeperException Ted?

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

Comment By: Patrick Hunt (phunt)
Date: 2008-05-01 14:37

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

Just me or is the latest patch missing the
com.yahoo.zookeeper.exceptions.* classes/package?

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

Comment By: Ted Dunning (tedunning)
Date: 2008-04-30 15:20

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

Here is a cleaner patch.  It leaves KeeperException out in the open and
makes it abstract.  This avoids an API change, but still gets the benefits
I was looking for.  I also addeda test case to check for messages and
sub-class of KE for all known codes.

File Added: 1948141-c.patch

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

Comment By: Ted Dunning (tedunning)
Date: 2008-04-23 15:50

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

I will provide another patch that does:

a) KeeperException back at the top and made abstract

b) All sub-types will be nested under KeeperException

c) A test that confirms all codes have messages and all sub-types have the
correct code.

These three changes will address most of the comments.  To wit:

- my desire to make sure that access is controlled

- Ben's desire to have a cross-check on error codes on the wire vs
exception types

- Patrick's desire not to have a new package

- everybody's desire to avoid API changes

I should get this out in a day or two.

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

Comment By: Patrick Hunt (phunt)
Date: 2008-04-23 11:00

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

Ok, that sounds reasonable to me. If this is the rational then we should
be sure to document it clearly -- at the very least the package.html for
exceptions package should make this clear.

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

Comment By: Benjamin Reed (breed)
Date: 2008-04-23 10:44

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

In general I agree with you Pat, I think for KeeperException the case is a
bit different. Except for corner cases KeeperException codes get translated
to an int and flow across wires. So, we need to have a good handle on the
possible exceptions and their corresponding code. This makes it useful to
centralize things a bit. Putting everything in an exception package in nice
in that way: we easily see the possible KeeperExceptions, so we can make
sure the codes are assigned correctly, we have messages for each code,
KeeperException.create() constructs the correct subclass, etc.

I'm not a big fan of nested classes. (Existing code notwithstanding :)
But, in this case, perhaps we should make the subclasses of KeeperException
nested classes. Each subclass is just one line of code anyway, so it saves
some clutter in the sourcetree.

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

Comment By: Patrick Hunt (phunt)
Date: 2008-04-23 09:23

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

I agree that there's not alot of cases, probably even none. Perhaps I'm
just stuck on what I consider "std idiom" for exceptions in java. My
personal preference is to mimic java APIs.

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

Comment By: Ted Dunning (tedunning)
Date: 2008-04-22 17:51

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

Patrick

I don't think it is a problem to have the exceptions near the code that
they come from.

I do think there are vanishingly few (== none?) use cases for a generic
KeeperException.  Since a code is required and it is part of the contract
that every valid code have a specific sub-class, then any KeeperException
should be a sub-type.  Perhaps a better way to enforce that is to just make
KeeperException abstract.

If there is consensus to do so, I can move the exceptions back to the top
level pretty easily.

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

Comment By: Patrick Hunt (phunt)
Date: 2008-04-22 17:22

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

Hi Ted, I like this change, it also struck me as odd to not have
sub-types.

However I find pushing the exceptions into their own package unnecessary.
I realize 
the idea is to hide the constructor of KeeperException, however I don't
think
this is required. We can document the constructor of KeeperException and
state that
creators should use a more specific specific subclass. In some cases it
may even 
be necessary for someone to instantiate a generic "KeeperException" (not a
sub). I
see this analogous to java.lang.Exception and it's subclasses. It can be
abused, but 
that can be weeded out during patch review.

I'd rather that the exceptions live "intermingled" with the non-exception
classes.
If QuorumPeer needs to throw a QuorumException should be in quorum
package, no? 
(if there was such a thing). This is similar to InputStream throwing a
IOException or one of it's more specific sub-types.

Regards, Patrick

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

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