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