> 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 > 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 > >
