----------------------------------------------------------- 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 > flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45 > > flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java > ab4c3de > flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java > 3c93921 > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java > PRE-CREATION > flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION > flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java > PRE-CREATION > > flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java > PRE-CREATION > flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION > flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION > flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java > PRE-CREATION > flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java > PRE-CREATION > flume-ng-sources/flume-scribe-source/pom.xml 588207d > > flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java > 401a7e4 > > flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java > 6bfa84c > > flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java > ccde51b > > flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java > 6190b11 > flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2 PRE-CREATION > flume-ng-sources/flume-scribe-source/src/main/thrift/scribe-source.thrift > PRE-CREATION > pom.xml faa67f9 > > Diff: https://reviews.apache.org/r/9284/diff/ > > > Testing > ------- > > Added unit tests for new code. > > > Thanks, > > Hari Shreedharan > >
