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

Switch to Threaded View
Flume, mail # dev - Review Request: FLUME-1896. Thrift Rpc Client


Copy link to this message
-
Re: Review Request: FLUME-1896. Thrift Rpc Client.
Hari Shreedharan 2013-02-09, 01:45


> On Feb. 9, 2013, 12:58 a.m., Mike Percy wrote:
> > flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2, line 1
> > <https://reviews.apache.org/r/9284/diff/8/?file=256978#file256978line1>
> >
> >     Not sure what this is for.

This is to insert the license into the generated code. See the pom.xml change.
> On Feb. 9, 2013, 12:58 a.m., Mike Percy wrote:
> > flume-ng-sdk/src/main/thrift/aslv2, line 1
> > <https://reviews.apache.org/r/9284/diff/8/?file=256987#file256987line1>
> >
> >     What is the point of this file?

same as above.
> On Feb. 9, 2013, 12:58 a.m., Mike Percy wrote:
> > flume-ng-sdk/src/main/thrift/flume.thrift, line 37
> > <https://reviews.apache.org/r/9284/diff/8/?file=256988#file256988line37>
> >
> >     What do you think about adding an additional health-check method here? Something like:
> >    
> >     Status areYouOk()

I don't think that is required. I don't see any value in adding that, if there is a failure, the next RPC will fail rightaway if it is a network issue, any others such as Channel issues cannot be predicted by such a health-check anyway.
> On Feb. 9, 2013, 12:58 a.m., Mike Percy wrote:
> > flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2, line 1
> > <https://reviews.apache.org/r/9284/diff/8/?file=256996#file256996line1>
> >
> >     This one too :)

same as above.
> On Feb. 9, 2013, 12:58 a.m., Mike Percy wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java, line 350
> > <https://reviews.apache.org/r/9284/diff/10/?file=257203#file257203line350>
> >
> >     Should this be remove()? Or we should do a null check.

Neither of these. There is a condition check (availableClient.isEmpty) on which a signal waits, so at this point, availableClient is guaranteed to be non-empty. Doing the condition.await and signal allows us to do lazy initialization.
> On Feb. 9, 2013, 12:58 a.m., Mike Percy wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java, line 362
> > <https://reviews.apache.org/r/9284/diff/10/?file=257203#file257203line362>
> >
> >     This is O(n) ... we could make this faster.

If checkedOutClients is small enough, this is pretty fast. That said, I think we can make checkedOutClients a LinkedHashSet I guess.
> On Feb. 9, 2013, 12:58 a.m., Mike Percy wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java, line 370
> > <https://reviews.apache.org/r/9284/diff/10/?file=257203#file257203line370>
> >
> >     This is O(n) ... can we get it down to O(1) or O(log n)?

same as above.
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9284/#review16373
-----------------------------------------------------------
On Feb. 8, 2013, 11:05 p.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9284/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2013, 11:05 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Added thrift rpc client. Detailed description posted on jira.
>
>
> This addresses bug FLUME-1896.
>     https://issues.apache.org/jira/browse/FLUME-1896
>
>
> Diffs
> -----
>
>   flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
>   flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION