Return-Path: X-Original-To: apmail-curator-dev-archive@minotaur.apache.org Delivered-To: apmail-curator-dev-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id B357A11C3A for ; Mon, 4 Aug 2014 18:48:41 +0000 (UTC) Received: (qmail 39309 invoked by uid 500); 4 Aug 2014 18:48:41 -0000 Delivered-To: apmail-curator-dev-archive@curator.apache.org Received: (qmail 39268 invoked by uid 500); 4 Aug 2014 18:48:41 -0000 Mailing-List: contact dev-help@curator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@curator.apache.org Delivered-To: mailing list dev@curator.apache.org Received: (qmail 39243 invoked by uid 99); 4 Aug 2014 18:48:41 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 04 Aug 2014 18:48:41 +0000 X-ASF-Spam-Status: No, hits=2.2 required=5.0 tests=HTML_MESSAGE,RCVD_IN_DNSWL_LOW,SPF_NEUTRAL X-Spam-Check-By: apache.org Received-SPF: neutral (athena.apache.org: local policy) Received: from [209.85.219.43] (HELO mail-oa0-f43.google.com) (209.85.219.43) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 04 Aug 2014 18:48:36 +0000 Received: by mail-oa0-f43.google.com with SMTP id i7so5367750oag.2 for ; Mon, 04 Aug 2014 11:48:15 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:message-id:in-reply-to :references:subject:mime-version:content-type; bh=LMvl1tL17jdTQI8ruKeJF+6VPHhF8W55XqaouaLthk0=; b=Zcnsg1Hib2NK1Ulv7pv88O41IqxIIlaBvlpyX/BSSgNZme5BEdGaaafrBt6qazMCdU S+48lcpxj/4D6zrcNkWNegMOa/A0eQLa4owBVKid+fI2sTlmlJQonvTpMb97E/DIZ50V kg2Y3WfotE9XEmIyhCh1HLyHAsUVBF3lvQNWav4vnk0p2Q8jwpOW52QZjCQbnU/Z3hWU UwVTZqeldkJel/JRwljkJWdOOBS3l8e+Y9G2oM2Nz2bhduwR/2puP/mB/1Y1YzUAAtlf GLyHGcQRh7zPtPQaAiGVbx6v5W6sNxFH6LW1jhBnozVDdEfAxovKgrhX7rqPNOOQlxn7 tKyQ== X-Gm-Message-State: ALoCoQmZ9LgHzBNZCXiDv0pHouoA7A7QDPj8mIxQzs2RJmIFTRUohfWtA+e6JQ0szytR+N0Ad/mL X-Received: by 10.60.97.40 with SMTP id dx8mr36036333oeb.27.1407178095684; Mon, 04 Aug 2014 11:48:15 -0700 (PDT) Received: from Jordans-MacBook-Pro.local ([190.140.208.165]) by mx.google.com with ESMTPSA id oj7sm40678790obc.19.2014.08.04.11.48.11 for (version=TLSv1.2 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Mon, 04 Aug 2014 11:48:15 -0700 (PDT) Date: Mon, 4 Aug 2014 13:47:55 -0500 From: Jordan Zimmerman To: dev@curator.apache.org, Cameron McKenzie , Mike Drob Cc: dev@curator.apache.org, vines@apache.org Message-ID: In-Reply-To: References:

Subject: Re: Exception throwing X-Mailer: Airmail (247) MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="53dfd56a_32794ff7_665b" X-Virus-Checked: Checked by ClamAV on apache.org --53dfd56a_32794ff7_665b Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline I already have a placeholder version 3.0 in Jira: https://issues.apache.org/jira/browse/CURATOR/fixforversion/12326664/=3F= selectedTab=3Dcom.atlassian.jira.jira-projects-plugin:version-summary-pan= el We could start putting together a Curator 3.0 wish list and iterate. -JZ =46rom:=C2=A0Mike Drob Reply:=C2=A0dev=40curator.apache.org > Date:=C2=A0August 4, 2014 at 9:29:14 AM To:=C2=A0Cameron McKenzie > Cc:=C2=A0dev=40curator.apache.org >, vines=40ap= ache.org > Subject:=C2=A0 Re: Exception throwing =20 I can see how it may be unappealing modifying everything in the library t= o =20 accommodate this. And I think I figured out why this matters (before I wa= s =20 suggesting it from mostly intuition). =20 Jordan, you posit that Curator assumes familiarity with ZooKeeper. In som= e =20 cases, that's warranted, but in others, I'm not so sure. I understand the= =20 basic ZooKeeper semantics - you can put and get data, it provides quorum = =20 guarantees, data is stored hierarchically, etc... However, I don't know t= he =20 ins and outs of the ZooKeeper API. I know that KeeperException is a thing= , =20 but I couldn't tell you which methods throw it, why they throw it, and wh= at =20 all the (enum=3F int=3F string=3F) codes are. Dealing directly with ZooKe= eper is =20 thorny=21 =20 Enter Curator=21 It gives me so much for free - retry policies, sanity =20 checking, a testing server for unit tests, it's great. Until I run into a= =20 KeeperException. That's a very jarring experience because I thought that = =20 Curator protected me from such nonsense. And it never advertised that =20 KeeperException was a possibility=21 My only indication was the 'throws =20 Exception' clause on most methods, which I have been taught by the librar= y =20 to dutifully ignore it and just fail fast. Had I seen a 'throws =20 KeeperException' declaration, I might have thought about what that means,= =20 but that information was not available to me. =20 Yes, it is unfortunate that ZooKeeper uses exceptions to return result =20 codes. That doesn't mean that we have to do the same. I would love to see= =20 CreateBuilder implement BackgroundPathAndBytesable instead of =20 String and for it to eat the KeeperException. Or maybe return a Stat. Jus= t =20 protect me from that exception. =20 Mike =20 On Sat, Aug 2, 2014 at 5:47 PM, Cameron McKenzie =20 wrote: =20 > I don't have a super strong view either way. If we were designing from = =20 > scratch I'd go with checked exceptions, just because that's how I prefe= r to =20 > code. =20 > =20 > Having said that, I'm not convinced that modifying the whole code base = at =20 > this point to add a CuratorException is worth the effort. =20 > =20 > =20 > On Sat, Aug 2, 2014 at 8:09 AM, Jordan Zimmerman < =20 > jordan=40jordanzimmerman.com> wrote: =20 > =20 >> Curator assumes familiarity with ZooKeeper. Of course, the docs should= be =20 >> improved where needed. Curator doesn=E2=80=99t hide any KeeperExceptio= ns except =20 >> where it advertises that it handles them - e.g. creatingParentsIfNeede= d(). =20 >> The fact that ZooKeeper uses exceptions to return result codes for cor= rect =20 >> behavior is the problem here. =20 >> =20 >> That said, I wrote nearly all of the Curator recipes and have never ha= d =20 >> an occasion where Curator=E2=80=99s use of Exception was a problem. =20 >> =20 >> -JZ =20 >> =20 >> =46rom: Mike Drob =20 >> Reply: dev=40curator.apache.org > =20 >> Date: August 1, 2014 at 5:05:04 PM =20 >> To: vines=40apache.org > =20 >> Cc: dev=40curator.apache.org > =20 >> Subject: Re: Exception throwing =20 >> =20 >> The set with version is basically a compareAndSet. java.util chooses t= o =20 >> implement this also with a 'return false' value for when some other th= read =20 >> got there first. I'll have to go back and look at the Curator API and = see =20 >> how these failures are currently communicated. =20 >> =20 >> =20 >> On =46ri, Aug 1, 2014 at 4:08 PM, John Vines wrot= e: =20 >> =20 >> > There's KeeperException.BADVERSION, which is when you =20 >> > setData().withVersion and the version in ZK changed vs. the version = seen =20 >> > prior. That one is pretty critical to support, IMO. The other cases = =20 >> around =20 >> > node existance can definitely be handled by the user, but given the = =20 >> > possibilities for races in distributed systems you still can't be =20 >> certain. =20 >> > But there could be user cases where they want to create a node and n= ot =20 >> fail =20 >> > if it exists or have a delete pass if the node was already deleted b= y =20 >> > something else (not sure if a flag was added for that case, I vaguel= y =20 >> > recall a discussion). =20 >> > =20 >> > =20 >> > On =46ri, Aug 1, 2014 at 5:04 PM, Mike Drob = wrote: =20 >> > =20 >> >> John, thanks for your input. So some of this is improper use of the= =20 >> API, =20 >> >> right=3F If you are attempting to create a node and it already exis= ts, =20 >> then =20 >> >> that can be an exceptional case. If you just want to make sure that= a =20 >> node =20 >> >> exists, regardless of who created it, then that's a case for a =20 >> different =20 >> >> API. Asserting that you created the node could be important - see t= he =20 >> >> distinction in ConcurrentMap between put() and putIfAbsent(). Then = =20 >> again, =20 >> >> failing to create the node because it already exists might not need= to =20 >> be =20 >> >> an exceptional case and simply warrants a 'return false' on the =20 >> method=3F Do =20 >> >> the other cases you mentioned have similar analogues=3F Maybe the e= nd =20 >> result =20 >> >> of what comes out of this is better docs. =20 >> >> =20 >> >> Mike =20 >> >> =20 >> >> =20 >> >> On =46ri, Aug 1, 2014 at 3:39 PM, John Vines w= rote: =20 >> >> =20 >> >>> It's not a matter of it being a bug, it's a matter of usability. =20 >> Because =20 >> >>> every single method just throws Exception it gives me, as a user, = =20 >> >>> absolutely zero inclination at writing time to figure out what sor= t of =20 >> >>> failures can happen. And the complete lack of javadocs compound th= is =20 >> >>> issue. =20 >> >>> This has been my biggest issue with Curator. =20 >> >>> =20 >> >>> Yes, there are some unrecoverable errors. But not all of them are,= =20 >> such =20 >> >>> as =20 >> >>> a subset of the KeeperExceptions around node state, security, and = =20 >> >>> versions. =20 >> >>> I could be sold on a split, where those type of items are exposed = and =20 >> >>> then =20 >> >>> the critical ones you keep mentioning are Runtime. But as much as = I =20 >> >>> dislike =20 >> >>> generic Exceptions for everything, forcing users to catch =20 >> >>> RuntimeExceptions =20 >> >>> to do proper exception handling for known and well defined excepti= ons =20 >> is =20 >> >>> an =20 >> >>> awful practice to put people though. =20 >> >>> =20 >> >>> =20 >> >>> On =46ri, Aug 1, 2014 at 3:49 PM, Jordan Zimmerman < =20 >> >>> jordan=40jordanzimmerman.com =20 >> >>> > wrote: =20 >> >>> =20 >> >>> > -1 (binding) =20 >> >>> > =20 >> >>> > If I could go back I=E2=80=99d remove all checked exceptions ent= irely. I =20 >> don=E2=80=99t =20 >> >>> > think there=E2=80=99s an objective answer here - it comes down t= o personal =20 >> >>> > preference, etc. I don=E2=80=99t see much value in touching near= ly every =20 >> file =20 >> >>> in =20 >> >>> > the library in order to do this. We=E2=80=99ve had maybe 2 or 3 = requests in =20 >> the =20 >> >>> > many years that Curator has exists. This suggests that the =20 >> overwhelming =20 >> >>> > majority accept the current exception semantics. If you can poin= t =20 >> to an =20 >> >>> > actual bug that this causes that would be helpful. =20 >> >>> > =20 >> >>> > -Jordan =20 >> >>> > =20 >> >>> > =46rom: Mike Drob =20 >> >>> > Reply: dev=40curator.apache.org > =20 >> >>> > Date: August 1, 2014 at 2:32:07 PM =20 >> >>> > To: dev=40curator.apache.org > =20 >> >>> > Subject: Exception throwing =20 >> >>> > =20 >> >>> > I'd like to revisit the discussion around always throwing Except= ion =20 >> in =20 >> >>> the =20 >> >>> > API. There were two JIRA issues - CURATOR-135 and CURATOR-29 - t= hat =20 >> >>> touch =20 >> >>> > on this subject, but I think there is a good conversation to be = had. =20 >> >>> > =20 >> >>> > I understand the suggestions that if an exception is thrown, we = are =20 >> in =20 >> >>> a =20 >> >>> > bad state, regardless of the type of exception. However, throwin= g =20 >> >>> Exception =20 >> >>> > comes with some unfortunate Java baggage... =20 >> >>> > =20 >> >>> > By declaring thrown Exception, we force consumers to also catch = =20 >> >>> > RuntimeExceptions instead of letting them propagate as they norm= ally =20 >> >>> would. =20 >> >>> > In some cases, the calling code may need to attempt roll-back =20 >> >>> semantics, or =20 >> >>> > retry outside of what Curator provides, or something else that w= e =20 >> >>> haven't =20 >> >>> > thought of. =20 >> >>> > =20 >> >>> > I'd like to propose replacing much of the thrown Exception metho= ds =20 >> with =20 >> >>> > CuratorException. This will still carry the connotation that it = =20 >> doesn't =20 >> >>> > matter what kind of exception we encounter, they're all bad. It = will =20 >> >>> also =20 >> >>> > be backwards compatible with the previous API, since users will = =20 >> still =20 >> >>> be =20 >> >>> > able to catch Exception in their calling code. And it has the =20 >> >>> advantage of =20 >> >>> > separating checked exceptions from unchecked ones, so that users= =20 >> don't =20 >> >>> > unintentionally catch something unrelated. =20 >> >>> > =20 >> >>> > Thoughts=3F =20 >> >>> > =20 >> >>> > I tried looking for more details behind the design decision to =20 >> always =20 >> >>> throw =20 >> >>> > Exception, but wasn't able to find them. If they're already =20 >> >>> documented, I'd =20 >> >>> > love to be pointed at the wiki or site, or a mailing list thread= =20 >> will =20 >> >>> do in =20 >> >>> > a pinch. =20 >> >>> > =20 >> >>> > Thanks, =20 >> >>> > Mike =20 >> >>> > =20 >> >>> =20 >> >> =20 >> >> =20 >> > =20 >> =20 > =20 > =20 --53dfd56a_32794ff7_665b--