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