curator-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Cameron McKenzie <mckenzie....@gmail.com>
Subject Re: Exception throwing
Date Sat, 02 Aug 2014 22:47:14 GMT
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 <jordan@jordanzimmerman.com
> 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 <madrob@cloudera.com>
> Reply: dev@curator.apache.org <dev@curator.apache.org>>
> Date: August 1, 2014 at 5:05:04 PM
> To: vines@apache.org <vines@apache.org>>
> Cc: dev@curator.apache.org <dev@curator.apache.org>>
> 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 <vines@apache.org> 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 <madrob@cloudera.com> 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 <vines@apache.org> 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 <
> >>> jordan@jordanzimmerman.com
> >>> > 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 <madrob@cloudera.com>
> >>> > Reply: dev@curator.apache.org <dev@curator.apache.org>>
> >>> > Date: August 1, 2014 at 2:32:07 PM
> >>> > To: dev@curator.apache.org <dev@curator.apache.org>>
> >>> > 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
> >>> >
> >>>
> >>
> >>
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message