This is good. How do we get there?
On Mon, Aug 4, 2014 at 9:36 AM, Jordan Zimmerman <[email protected] > wrote: > Just > protect me from that exception. > > I agree with this. > I’d like to see Curator move to unchecked exceptions everywhere. For > “recoverable exceptions”, I’d like to see something like this (BTW - I SO > wish I did this initially): have the various forPath() methods return a > class (rather than byte[], etc.) like: > > public interface CuratorResult > { > public byte[] getBytes(); > > public CuratorResultType getResultType(); > } > > public enum CuratorResultType > { > SUCCESS, > NO_NODE, > NODE_EXISTS, > BAD_VERSION > … etc > } > > To me, this is FAR superior than the recoverable exceptions. It would also > make the APIs a lot easier to use. You could do: > > CuratorResult result = client.create().inBackground().forPath(myPath); > if ( result.getResultType() == SUCCESS ) > { > // etc. > } > else > { > // etc. > } > > From: Mike Drob <[email protected]> <[email protected]> > Reply: [email protected] <[email protected]>> > <[email protected]> > Date: August 4, 2014 at 9:29:14 AM > To: Cameron McKenzie <[email protected]>> <[email protected]> > Cc: [email protected] <[email protected]>> > <[email protected]>, [email protected] <[email protected]>> > <[email protected]> > Subject: Re: Exception throwing > > I can see how it may be unappealing modifying everything in the library to > accommodate this. And I think I figured out why this matters (before I was > suggesting it from mostly intuition). > > Jordan, you posit that Curator assumes familiarity with ZooKeeper. In some > cases, that's warranted, but in others, I'm not so sure. I understand the > basic ZooKeeper semantics - you can put and get data, it provides quorum > guarantees, data is stored hierarchically, etc... However, I don't know > the > ins and outs of the ZooKeeper API. I know that KeeperException is a thing, > but I couldn't tell you which methods throw it, why they throw it, and > what > all the (enum? int? string?) codes are. Dealing directly with ZooKeeper is > thorny! > > Enter Curator! It gives me so much for free - retry policies, sanity > checking, a testing server for unit tests, it's great. Until I run into a > KeeperException. That's a very jarring experience because I thought that > Curator protected me from such nonsense. And it never advertised that > KeeperException was a possibility! My only indication was the 'throws > Exception' clause on most methods, which I have been taught by the library > to dutifully ignore it and just fail fast. Had I seen a 'throws > KeeperException' declaration, I might have thought about what that means, > but that information was not available to me. > > Yes, it is unfortunate that ZooKeeper uses exceptions to return result > codes. That doesn't mean that we have to do the same. I would love to see > CreateBuilder implement BackgroundPathAndBytesable<Boolean> instead of > String and for it to eat the KeeperException. Or maybe return a Stat. Just > protect me from that exception. > > Mike > > > > On Sat, Aug 2, 2014 at 5:47 PM, Cameron McKenzie <[email protected]> > wrote: > > > I don't have a super strong view either way. If we were designing from > > scratch I'd go with checked exceptions, just because that's how I prefer > to > > code. > > > > Having said that, I'm not convinced that modifying the whole code base > at > > this point to add a CuratorException is worth the effort. > > > > > > On Sat, Aug 2, 2014 at 8:09 AM, Jordan Zimmerman < > > [email protected]> wrote: > > > >> Curator assumes familiarity with ZooKeeper. Of course, the docs should > be > >> improved where needed. Curator doesn’t hide any KeeperExceptions except > >> where it advertises that it handles them - e.g. > creatingParentsIfNeeded(). > >> The fact that ZooKeeper uses exceptions to return result codes for > correct > >> behavior is the problem here. > >> > >> That said, I wrote nearly all of the Curator recipes and have never had > >> an occasion where Curator’s use of Exception was a problem. > >> > >> -JZ > >> > >> From: Mike Drob <[email protected]> > >> Reply: [email protected] <[email protected]>> > >> Date: August 1, 2014 at 5:05:04 PM > >> To: [email protected] <[email protected]>> > >> Cc: [email protected] <[email protected]>> > >> Subject: Re: Exception throwing > >> > >> The set with version is basically a compareAndSet. java.util chooses to > >> implement this also with a 'return false' value for when some other > thread > >> got there first. I'll have to go back and look at the Curator API and > see > >> how these failures are currently communicated. > >> > >> > >> On Fri, Aug 1, 2014 at 4:08 PM, John Vines <[email protected]> wrote: > >> > >> > There's KeeperException.BADVERSION, which is when you > >> > setData().withVersion and the version in ZK changed vs. the version > seen > >> > prior. That one is pretty critical to support, IMO. The other cases > >> around > >> > node existance can definitely be handled by the user, but given the > >> > possibilities for races in distributed systems you still can't be > >> certain. > >> > But there could be user cases where they want to create a node and > not > >> fail > >> > if it exists or have a delete pass if the node was already deleted by > >> > something else (not sure if a flag was added for that case, I vaguely > >> > recall a discussion). > >> > > >> > > >> > On Fri, Aug 1, 2014 at 5:04 PM, Mike Drob <[email protected]> > wrote: > >> > > >> >> John, thanks for your input. So some of this is improper use of the > >> API, > >> >> right? If you are attempting to create a node and it already exists, > >> then > >> >> that can be an exceptional case. If you just want to make sure that > a > >> node > >> >> exists, regardless of who created it, then that's a case for a > >> different > >> >> API. Asserting that you created the node could be important - see > the > >> >> distinction in ConcurrentMap between put() and putIfAbsent(). Then > >> again, > >> >> failing to create the node because it already exists might not need > to > >> be > >> >> an exceptional case and simply warrants a 'return false' on the > >> method? Do > >> >> the other cases you mentioned have similar analogues? Maybe the end > >> result > >> >> of what comes out of this is better docs. > >> >> > >> >> Mike > >> >> > >> >> > >> >> On Fri, Aug 1, 2014 at 3:39 PM, John Vines <[email protected]> > wrote: > >> >> > >> >>> It's not a matter of it being a bug, it's a matter of usability. > >> Because > >> >>> every single method just throws Exception it gives me, as a user, > >> >>> absolutely zero inclination at writing time to figure out what sort > of > >> >>> failures can happen. And the complete lack of javadocs compound > this > >> >>> issue. > >> >>> This has been my biggest issue with Curator. > >> >>> > >> >>> Yes, there are some unrecoverable errors. But not all of them are, > >> such > >> >>> as > >> >>> a subset of the KeeperExceptions around node state, security, and > >> >>> versions. > >> >>> I could be sold on a split, where those type of items are exposed > and > >> >>> then > >> >>> the critical ones you keep mentioning are Runtime. But as much as I > >> >>> dislike > >> >>> generic Exceptions for everything, forcing users to catch > >> >>> RuntimeExceptions > >> >>> to do proper exception handling for known and well defined > exceptions > >> is > >> >>> an > >> >>> awful practice to put people though. > >> >>> > >> >>> > >> >>> On Fri, Aug 1, 2014 at 3:49 PM, Jordan Zimmerman < > >> >>> [email protected] > >> >>> > wrote: > >> >>> > >> >>> > -1 (binding) > >> >>> > > >> >>> > If I could go back I’d remove all checked exceptions entirely. I > >> don’t > >> >>> > think there’s an objective answer here - it comes down to > personal > >> >>> > preference, etc. I don’t see much value in touching nearly every > >> file > >> >>> in > >> >>> > the library in order to do this. We’ve had maybe 2 or 3 requests > in > >> the > >> >>> > many years that Curator has exists. This suggests that the > >> overwhelming > >> >>> > majority accept the current exception semantics. If you can point > >> to an > >> >>> > actual bug that this causes that would be helpful. > >> >>> > > >> >>> > -Jordan > >> >>> > > >> >>> > From: Mike Drob <[email protected]> > >> >>> > Reply: [email protected] <[email protected]>> > >> >>> > Date: August 1, 2014 at 2:32:07 PM > >> >>> > To: [email protected] <[email protected]>> > >> >>> > Subject: Exception throwing > >> >>> > > >> >>> > I'd like to revisit the discussion around always throwing > Exception > >> in > >> >>> the > >> >>> > API. There were two JIRA issues - CURATOR-135 and CURATOR-29 - > that > >> >>> touch > >> >>> > on this subject, but I think there is a good conversation to be > had. > >> >>> > > >> >>> > I understand the suggestions that if an exception is thrown, we > are > >> in > >> >>> a > >> >>> > bad state, regardless of the type of exception. However, throwing > >> >>> Exception > >> >>> > comes with some unfortunate Java baggage... > >> >>> > > >> >>> > By declaring thrown Exception, we force consumers to also catch > >> >>> > RuntimeExceptions instead of letting them propagate as they > normally > >> >>> would. > >> >>> > In some cases, the calling code may need to attempt roll-back > >> >>> semantics, or > >> >>> > retry outside of what Curator provides, or something else that we > >> >>> haven't > >> >>> > thought of. > >> >>> > > >> >>> > I'd like to propose replacing much of the thrown Exception > methods > >> with > >> >>> > CuratorException. This will still carry the connotation that it > >> doesn't > >> >>> > matter what kind of exception we encounter, they're all bad. It > will > >> >>> also > >> >>> > be backwards compatible with the previous API, since users will > >> still > >> >>> be > >> >>> > able to catch Exception in their calling code. And it has the > >> >>> advantage of > >> >>> > separating checked exceptions from unchecked ones, so that users > >> don't > >> >>> > unintentionally catch something unrelated. > >> >>> > > >> >>> > Thoughts? > >> >>> > > >> >>> > I tried looking for more details behind the design decision to > >> always > >> >>> throw > >> >>> > Exception, but wasn't able to find them. If they're already > >> >>> documented, I'd > >> >>> > love to be pointed at the wiki or site, or a mailing list thread > >> will > >> >>> do in > >> >>> > a pinch. > >> >>> > > >> >>> > Thanks, > >> >>> > Mike > >> >>> > > >> >>> > >> >> > >> >> > >> > > >> > > > > > >
