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.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9284/#review16274
-----------------------------------------------------------
Thanks Hari! Looks good! A few comments below. I'll do another review later.
flume-ng-legacy-sources/flume-thrift-source/pom.xml
<https://reviews.apache.org/r/9284/#comment34729>

    If this fails we do we want to print the error and then exit?

flume-ng-legacy-sources/flume-thrift-source/pom.xml
<https://reviews.apache.org/r/9284/#comment34730>

    same here?

flume-ng-sdk/pom.xml
<https://reviews.apache.org/r/9284/#comment34731>

    Same questions on this script

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34732>

    nit: Looks like the variables in the constructor can be set to final

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34733>

    nit: it's nice to have this up at the top of the class

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34734>

    Is this meant to be e.getCause() instance of?
    
    I don't follow the layers of instanceof.

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34735>

    Any throwable not matching the above is turned into an EventDeliveryException including Error and RuntimeException. I don't think we should do that for runtime errors.

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34736>

    Not a huge deal, but we do this "Exception follows" messages all over flume. I don't really see a purpose in that part of the text?

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34737>

    Same exception stuff as above

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34738>

    remove sysout

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34741>

    Looks like there are more than two stati, if so I think we should include the status in the error message.

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34739>

    remove sysout

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34740>

    converting all throwables to FlumeException should be handled better
- Brock Noland
On Feb. 7, 2013, 3:31 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9284/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2013, 3:31 a.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
>   flume-ng-sdk/pom.xml ab066d5