Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Plain View
Accumulo, mail # dev - Review Request 15166: ACCUMULO-802 Tablespaces


+
Christopher Tubbs 2013-11-01, 02:01
+
John Vines 2013-11-01, 22:19
+
Christopher Tubbs 2013-11-08, 01:08
+
John Vines 2013-11-08, 19:58
+
Sean Hickey 2013-11-10, 00:50
+
Christopher Tubbs 2013-11-14, 21:40
+
Christopher Tubbs 2013-11-14, 21:07
+
Christopher Tubbs 2013-11-18, 22:09
+
John Vines 2013-11-29, 18:59
+
Christopher Tubbs 2013-12-04, 21:44
+
Christopher Tubbs 2013-12-04, 21:41
+
John Vines 2013-12-04, 22:31
+
Christopher Tubbs 2013-12-04, 21:26
+
Christopher Tubbs 2013-12-04, 21:37
+
keith@... 2013-11-08, 19:51
+
Keith Turner 2013-11-08, 19:58
+
Keith Turner 2013-11-08, 23:10
+
Sean Hickey 2013-11-10, 01:16
+
Christopher Tubbs 2013-12-04, 21:54
+
Christopher Tubbs 2013-12-04, 23:44
+
Mike Drob 2013-11-08, 19:08
+
Mike Drob 2013-11-08, 19:27
+
John Vines 2013-11-08, 19:58
+
Christopher Tubbs 2013-11-08, 20:10
+
Josh Elser 2013-11-08, 22:16
+
John Vines 2013-11-08, 22:01
+
John Vines 2013-11-08, 21:55
+
Mike Drob 2013-11-08, 21:52
+
Mike Drob 2013-11-08, 21:52
+
Sean Hickey 2013-11-10, 00:57
+
Christopher Tubbs 2013-11-18, 22:13
Copy link to this message
-
Re: Review Request 15166: ACCUMULO-802 Tablespaces
John Vines 2013-11-18, 22:00


> On Nov. 8, 2013, 7:08 p.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/TableNamespaceExistsException.java, line 24
> > <https://reviews.apache.org/r/15166/diff/1/?file=376077#file376077line24>
> >
> >     I would like to see this (and the other two exceptions) all extend RuntimeException. I think that since these are all conditions that can be checked for before attempting operations, it is unnecessarily burdensome on the client to force additional checked Exceptions on them.
>
> John Vines wrote:
>     A million times no. Do NOT force clients to catch RuntimeExceptions to handle your error codes.
>
> Christopher Tubbs wrote:
>     I agree they should not be RuntimeExceptions.
>
> Mike Drob wrote:
>     You're already forcing clients to catch the exceptions when you declare them as checked exceptions. And there is an exists method on operations, which is completely pointless with the checked exceptions. Consider the following code snippets:
>    
>     // Can write this if TableNamespaceExistsException is Runtime
>     if (! namespaceOperations.exists("foo"))
>       namespaceOperations.create("foo")
>    
>     // If it is checked, then you have to write
>     if (! namespaceOperations.exists("foo")) {
>       try {
>         namespaceOperations.create("foo")
>       } catch (TableNamespaceExistsException tnee) {
>         // log? nothing we can do anyway.
>       }
>     }
>    
>     // Or even worse, skip the existence check entirely and just try to create...
>     // With this sample, what is even the point of having an exists() method?
>     try {
>       namespaceOperations.create("foo")
>     } catch (Exception e) {
>       // log? nothing we can do anyway.
>     }
>    
>    
>     Now, my preferred snippet (#1) _is_ susceptible to a potential race condition, but I think the solution is a more serious API redesign. Maybe have create return a boolean instead of throwing, similar to Collection.add(). Or maybe add a createIfAbsent() method, similar to ConcurrentHashMap.putIfAbsent().
>    
>     There's lots of ways to do this, but forcing users to catch clunky exceptions is one of my least favorite parts of Java. Also see Effective Java, Item 59.
>
> John Vines wrote:
>     Catching a well defined exception is a hell of a lot different then catching RuntimeException. Runtime exceptions should be reserved for errors where things are fundamentally broken, not because you're too lazy to write a catch clause.
>
> John Vines wrote:
>     And while I do like the idea of a boolean for operation returns, they don't provide any input as to why they fail, which makes programmatic handling worse. You're going to have drastically different behavior if a table creation fails because it already exists vs. if you don't have permissions for it vs. the cluster is on fire
>
> Josh Elser wrote:
>     I'm agree with John here. First, we already have the semantics of throwing a named, non-Runtime exception with Table operations -- I see no reason to treat namespaces differently than tables. Second, you're making a sweeping assumption that all clients will do nothing when a namespace already exists. You can't possibly know this to be true.
>    
>     Yes, it's more verbose, but this *is* Java which has never been known for its subtlety. I'd much rather have a few more lines of code than withhold error information from the client.
>
> Christopher Tubbs wrote:
>     After some consideration, I think I'm inclined to agree with Mike that NamespaceNotFoundException should be a RuntimeException... because it represents a programmer error, of the type described by Joshua Bloch in Effective Java, rather than an easily recoverable situation. It's not that I expect clients to do nothing if it already exists... it's that I expect clients shouldn't be getting into this situation in the first place. I'm not 100% convinced that it applies to NamespaceExistsException... right now I'm inclined to think that it should be checked, because it's behavior should be considered independently to the exists() method.

Again, STRONGLY disagree. We're dealing with a distributed system, and as such could have clients racing one another to check and then create the namespace if it does not exist. These are perfectly valid conditions that can be raced against one another and cause a failure in spite of however much prechecking is done. So there are cases where it is NOT necessarily a programmer error and should NOT be a runtime exception.
- John
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15166/#review28554
On Nov. 1, 2013, 2:01 a.m., Christopher Tubbs wrote:
+
Christopher Tubbs 2013-11-18, 21:45
+
Christopher Tubbs 2013-11-18, 21:29
+
keith@... 2013-11-08, 20:11