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

Switch to Plain View
Accumulo, mail # dev - Review Request: Patch submitted by Chris McCubbin to add thrift proxy to Accumulo


+
keith@... 2012-11-07, 20:33
+
keith@... 2012-11-07, 21:14
+
keith@... 2012-11-07, 21:55
Copy link to this message
-
Re: Review Request: Patch submitted by Chris McCubbin to add thrift proxy to Accumulo
Chris McCubbin 2012-11-07, 21:40


> On Nov. 7, 2012, 9:14 p.m., kturner wrote:
> > /trunk/proxy/src/main/thrift/proxy.thrift, line 72
> > <https://reviews.apache.org/r/7936/diff/1/?file=186436#file186436line72>
> >
> >     can anything be done to make methods shorter?  Is it possible to get rid of all of the tableOperation_ and instanceOperation_ prefixes?  Does having different services (ie. AccumuloInstanceProxy, AccumloTableOpProxy) force us to run those services on separate ports?

Different services must be run on separate ports, yes. That was the reason I included the prefixes. See this really old thrift ticket where they basically say they aren't going to change that: https://issues.apache.org/jira/browse/THRIFT-66
> On Nov. 7, 2012, 9:14 p.m., kturner wrote:
> > /trunk/proxy/src/main/thrift/proxy.thrift, line 140
> > <https://reviews.apache.org/r/7936/diff/1/?file=186436#file186436line140>
> >
> >     Why not a list of Mutations?

TMutation uses serialized ColumnUpdates which would be very confusing to generate. I could make a proxy Mutation structure but this is fairly redundant with a list of TKeyValues.

The first cut at the implementation of insert grouped the keyvalues into Mutations by row. I took that out because it wasn't more performant than one Mutation per keyvalue. Adam just brought up a good point that we might want the assumption of atomicity for these entries, so I could change the server back to doing row grouping.
> On Nov. 7, 2012, 9:14 p.m., kturner wrote:
> > /trunk/proxy/src/main/thrift/proxy.thrift, line 133
> > <https://reviews.apache.org/r/7936/diff/1/?file=186436#file186436line133>
> >
> >     the batch scanner will need to be closed in some way.  I did not see this being done on the server side.
> >    
> >     The thrift API could provide a close method.  Or the server side could time out idle batch scanner and close them.

Good point. I had this on my list to do but didn't get it in there.
> On Nov. 7, 2012, 9:14 p.m., kturner wrote:
> > /trunk/proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java, line 540
> > <https://reviews.apache.org/r/7936/diff/1/?file=186420#file186420line540>
> >
> >     sync on batchScanner before checking if its null

Good catch
> On Nov. 7, 2012, 9:14 p.m., kturner wrote:
> > /trunk/proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java, line 543
> > <https://reviews.apache.org/r/7936/diff/1/?file=186420#file186420line543>
> >
> >     In past experience with Thrift throwing a TException in server side code woudl result in client seeing nothing or seeing something like an IOException instead of a TApplicationException.   But this was a while ago, not sure what current behavior of thrift is.

This isn't the case currently. An exception is definitely generated on the client side.
> On Nov. 7, 2012, 9:14 p.m., kturner wrote:
> > /trunk/proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java, line 547
> > <https://reviews.apache.org/r/7936/diff/1/?file=186420#file186420line547>
> >
> >     If user does not scan until end, then will scanner always be present in scannerMap and iteratorMap?  If so this could lead to memory leaks.  I think its a commom use case that users do not read all data from a scanner.

Yes, this should be addressed.
- Chris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7936/#review13220
-----------------------------------------------------------
On Nov. 7, 2012, 8:33 p.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7936/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2012, 8:33 p.m.)
>
>
> Review request for accumulo.
>
>
> Description
> -------
>
> Patch submitted by Chris McCubbin to add thrift proxy to Accumulo
>
>
> This addresses bug ACCUMULO-482.
>     https://issues.apache.org/jira/browse/ACCUMULO-482
+
Chris McCubbin 2012-11-07, 21:44
+
keith@... 2012-11-07, 21:41
+
Chris McCubbin 2012-11-07, 21:55
+
keith@... 2012-11-07, 22:05
+
keith@... 2012-11-07, 21:44