curator-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mike Drob <mad...@cloudera.com>
Subject Re: Exception throwing
Date Mon, 04 Aug 2014 14:58:36 GMT
This is good. How do we get there?


On Mon, Aug 4, 2014 at 9:36 AM, Jordan Zimmerman <jordan@jordanzimmerman.com
> 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 <madrob@cloudera.com> <madrob@cloudera.com>
> Reply: dev@curator.apache.org <dev@curator.apache.org>>
> <dev@curator.apache.org>
> Date: August 4, 2014 at 9:29:14 AM
> To: Cameron McKenzie <mckenzie.cam@gmail.com>> <mckenzie.cam@gmail.com>
> Cc: dev@curator.apache.org <dev@curator.apache.org>>
> <dev@curator.apache.org>, vines@apache.org <vines@apache.org>>
> <vines@apache.org>
> 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 <mckenzie.cam@gmail.com>
> 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 <
> > 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