Not sure where we left off or what action items came out of this but from Kanak and Kishore's responses I suggest we consider taking an API based approach. I wanted to have some structure to this so that we can approach defining APIs and exceptions together in a controlled manner.
I suggest we have a separate project API that way its not muddled with the implementation(s) and its can be delivered to the users of Helix. The API will dictate the surface area exposed by Helix, that surface area can then be versioned and should follow a lifecycle (i.e. deprecation etc) so that subsequent releases can clearly identify what changed by leveraging annotations like @since note when it changed. Starting to focus on API as a separate project then allows us to selectively move packages, interfaces into API after we whet out what features are to be exposed to the user(s). We can add exceptions in the correct places (methods/classes) but lets do this through the lens of an API, lets look at each of the APIs that need to be exposed and find what exceptions make sense and throw them correctly at the right level. We may choose to take one or more approaches listed above through examples by Kanak but like I said lets do this through the lens of an API. I think once we have such a project then we can start to realize it through a review process which will make sure the right interfaces, methods and exceptions are exposed and does not get cluttered. What do you guys think? Sandeep On Thu, Feb 20, 2014 at 8:51 AM, kishore g <[email protected]> wrote: > Good thread, this is really valuable. Thanks Sandeep for taking the > initiative. I like the idea of having one package where we define all > exceptions. > > my preference > -- good to have baseclass for all exceptions. > -- too many levels will cause problems down the line. > -- Good to know why something failed, but I am fine using the exception > message to communicate that. > > > Good references Kanak. In general, as a user its hard to write code to > handle too many exception types. We should try to minimize the number of > exceptions. Here is how I would think, if we add this exception type is > there something a user can programmatically do by knowing the type of > exception. If there is a use case, then it certainly makes sense to have a > concrete exception of that type (AlreadyExistsException is definitely > something that user can programmatically handle). If the only motivation is > to help user debug or know why it failed, I would use the exception message > to communicate additional information. > > We should probably file a jira to track this change. > > Another thread I would like to start is the api. I think we need to revisit > the api's and make it more intuitive. In 0.7.0, we tried to maintain > compatibility with older api's. I see that its causing more pain and > confusion. I would rather spend more time and do the right set of changes > even if it breaks compatibility in the logical layer. However we should not > break the compatibility in physical layer, existing znode structure should > be maintained. > > Thoughts? > > thanks, > Kishore G > > > > > > > > > > > > > > > On Wed, Feb 19, 2014 at 11:02 PM, Kanak Biscuitwala > <[email protected]>wrote: > >> Hi Sandeep, >> >> Yes, and I definitely appreciate you starting this thread. It's something >> that Helix doesn't really have right now and it's absolutely necessary for >> intuitive APIs. >> >> For reference, here are how some other projects tackle this (not >> necessarily Helix-related): >> Cassandra: >> https://github.com/apache/cassandra/tree/trunk/src/java/org/apache/cassandra/exceptions(broken >> into themes like ConfigException, and then subclasses with the type >> -- similar to what you're suggesting)Hive: >> https://github.com/apache/hive(one exception per package, roughly)ZooKeeper: >> https://github.com/apache/zookeeper/blob/trunk/src/java/main/org/apache/zookeeper/KeeperException.java?source=c(all >> in one wrapper class, flat structure describing on the type of >> failure)Yarn: >> https://github.com/apache/hadoop-common/tree/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/exceptions(flat, >> class only describes the error type) >> Personally, I think a good scheme is to have a hierarchy, but with maybe >> only two levels: (1) the scope of the error, e.g. ConfigException, and (2) >> the cause, e.g. ConfigExistsException. >> Kanak >> >> ---------------------------------------- >> > Date: Wed, 19 Feb 2014 22:24:12 -0800 >> > Subject: Re: Exceptions in Helix >> > From: [email protected] >> > To: [email protected] >> > >> > Hi Kanak, >> > >> > Like I said the first stab was to start a conversation. So what you are >> > pointing at is defining an exception based on the API invoked. So a >> > NodeAlreadyExistsException can be thrown on an addNode API but that >> > exception should specialization of the ConfigException to clearly >> indicate >> > that the set of exceptions happen during configuration of the >> nodes/cluster. >> > >> > Sandeep >> > >> > >> > On Wed, Feb 19, 2014 at 10:08 PM, Kanak Biscuitwala <[email protected] >> >wrote: >> > >> > > My concern with this hierarchy is that it does a good job of describing >> > > what fails, but doesn't really hint at why. For instance, when trying >> to >> > > create a cluster that already exists, we would get something like a >> > > ClusterConfigException, and need to read javadocs to understand how it >> > > could be thrown. An alternative is to create exceptions based on error >> > > type, like ExistsException or something. >> > > >> > > Anyone else have thoughts? >> > > >> > > > Date: Wed, 19 Feb 2014 21:48:00 -0800 >> > > > Subject: Exceptions in Helix >> > > > From: [email protected] >> > > > To: [email protected] >> > > > >> > > > Hi guys, >> > > > >> > > > I sent out a mail last night (on the users maillist) that currently >> > > > Instance configuration retrieval from HelixAdmin throws >> HelixException >> > > > which is a runtime exception. >> > > > >> > > > I did not want to create a whole exception hierarchy for every API >> given >> > > my >> > > > narrow scope of understanding but wanted to illustrate an exception >> > > > hierarchy focussed on the above aspect and maybe influence an >> exception >> > > > hierarchy design which I am hoping someone who has a better grasp on >> the >> > > > entire codebase can pick and flesh out further. >> > > > >> > > > I looked at >> > > https://cwiki.apache.org/confluence/display/HELIX/API+Redesign and >> > > > saw the configuration hierarchy. >> > > > >> > > > So given that context here is my first stab at the Helix >> configuration >> > > > exception hierarchy. >> > > > >> > > > >> > > > - *HelixException //**This is base for all checked exceptions in >> > > Helix* >> > > > - *HelixConfigException* extends HelixException //This represents >> > > > checked exception hierarchy for configuration exceptions >> > > > - *HelixResourceConfigException* extends HelixConfigException >> > > > - *HelixStateModelConfigException* extends HelixConfigException >> > > > - *HelixInstanceConfigException* extends HelixConfigException >> > > > //Config exception specific to instances being added >> > > > - *HelixSpectatorConfigException* extends >> > > > HelixNodeConfigException //Specialized exceptions for types >> > > *if >> > > > applicable or necessary* >> > > > - *HelixParticipantConfigException* extends >> > > > HelixNodeConfigException //Specialized exceptions for types >> > > *if >> > > > applicable or necessary* >> > > > - *HelixControllerConfigException* extends >> > > > HelixNodeConfigException //Specialized exceptions for types >> > > *if >> > > > applicable or necessary* >> > > > - Other Checked Exceptions which are non-config but identify >> > > other >> > > > aspects of Helix which represent distinct subsets of >> > > functionality, like >> > > > the above represents configuration part. >> > > > - *HelixRuntimeException //**Base for all exceptions from which Helix >> > > > cannot recover* >> > > > - All derived classes which are failures which cannot be recovered >> > > > from >> > > > >> > > > Again I am throwing this out to start a conversation and maybe >> influence >> > > a >> > > > design approach for exceptions in future releases. I apologize for >> not >> > > > spending time on drawing a neat diagram, I decided to something out >> > > sooner >> > > > rather than wait until I got together a pretty diagram. >> > > > >> > > > Hopefully, you guys get an idea of what I am trying to convey. Feel >> free >> > > to >> > > > send questions if you want clarity on my suggestion, but please >> recognize >> > > > that its only been less than a week that I have looked at Helix so I >> hope >> > > > you won't expect me to chalk up the entire hierarchy :-) I think one >> of >> > > you >> > > > guys who has worked on the codebase for sometime may be better >> suited. I >> > > > can certainly help review and/or fine-tune and in the process learn >> Helix >> > > > internals a bit more. >> > > > >> > > > Thanks, >> > > > >> > > > Sandeep >> > > >> > > >>
