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

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


Copy link to this message
-
Re: Review Request 15166: ACCUMULO-802 Tablespaces
John Vines 2013-11-29, 18:59


> On Nov. 1, 2013, 10:19 p.m., John Vines 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>
> >
> >     If we make TableNamespaceExistException extend AccumuloExtension we can get around a lot of api compatibility issues
>
> Christopher Tubbs wrote:
>     That doesn't follow the existing pattern with TableExistsException, and the Exception does not appear to be thrown in any existing API. To what API-compatibility issues are you referring?
>
> John Vines wrote:
>     There is an issue with TableNamespaceNotFoundException and I think all 3 of these exceptions should be in the same family, i.e. implemented by similar means.
>
> Sean Hickey wrote:
>     Like Christopher pointed out, I was just following what Tables did with their exceptions. If you think that should be different, then we might want to change the table exceptions too.
>
> Christopher Tubbs wrote:
>     I think this can be addressed later. I think that where the rare places where a TableNamespaceNotFoundException is wrapped with a RuntimeException, it is sufficient, because namespace operations should be rare, and namespaces can generally be stable. It's not that I think it shouldn't be addressed... I just think it's not a priority.

I disagree. Wrapping legitimate exceptions in RTEs A. lead to shitty user APIs and B. lead to APIs that will have to be deprecated in order to implement them properly
> On Nov. 1, 2013, 10:19 p.m., John Vines wrote:
> > core/src/main/java/org/apache/accumulo/core/client/admin/TableNamespaceOperationsImpl.java, line 335
> > <https://reviews.apache.org/r/15166/diff/1/?file=376084#file376084line335>
> >
> >     RuntimeException should not be used here

How was this fixed?
> On Nov. 1, 2013, 10:19 p.m., John Vines wrote:
> > server/base/src/main/java/org/apache/accumulo/server/client/ClientServiceHandler.java, line 431
> > <https://reviews.apache.org/r/15166/diff/1/?file=376130#file376130line431>
> >
> >     This error could cause confusion when you have a table in the default namespace with the same name as a namespace (if that's possible?)
>
> Christopher Tubbs wrote:
>     I don't think so, not since the "why" string explicitly says "Could not find table namespace while getting configuration."
>
> John Vines wrote:
>     Resorting to parsing strings to determine errors programmatically is pretty crummy for end users. Also, looking through this code path shows that TableNamespaceOperationsImpl#getProperties doesn't work right since it looks for the type NOTFOUND, which this error isn't setting. Perhaps we should make TableOperation a combination of Table and TableNamespace options in order to support backwards compatibility while maintaining a solid set of easily decipherable errors.
>
> Sean Hickey wrote:
>     Well, if you really want to be able to easily find it programmatically, it might be best to just make a new type of exception (like ThriftNamespaceOperationException or something) because all the namespace stuff is new and won't break any backwards compatibility. I was just trying to reuse some existing stuff, especially with the thrift calls because I was brand new to thrift.
>
> Christopher Tubbs wrote:
>     I don't think users should parse error messages. But, your initial comment wasn't about being able to programmatically determine the error... it was about potential for confusion. My comment was in that regard.
>    
>     I don't want to create a bunch of new thrift exceptions at this point... but it's an option. For now, I can fix the NOTFOUND oversight, so a better exception is thrown in the client API.

This also seems to be marked as resolved by not fixed in the 802 branch...
> On Nov. 1, 2013, 10:19 p.m., John Vines wrote:
> > server/base/src/main/java/org/apache/accumulo/server/conf/TableNamespaceConfWatcher.java, line 29
> > <https://reviews.apache.org/r/15166/diff/1/?file=376133#file376133line29>

ACCUMULO-1871

I'm suggesting instead of replicating every single Table operation and table permission call to reflect table namespaces that we change the table operations to do both the individual table and the table namespace lookup. It should be adding a _hasTableNamespacePermission and changing hasTablePermission to call both _hasTablePermission and _hasTableNamespacePermission (or changing _hasTablePermission to do both the Table and TableNamespace checks, I could go either way currently). Then every single can* operation can remain unchanged. This is critical because any other can* checks could easily slip up the logic or miss a namespace, which would not be good.
- John
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15166/#review28029
On Nov. 1, 2013, 2:01 a.m., Christopher Tubbs wrote: