> On Feb. 10, 2013, 9:11 p.m., Brock Noland wrote: > > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java, line > > 337 > > <https://reviews.apache.org/r/9284/diff/10/?file=257203#file257203line337> > > > > Would a small amount of synchronization and a BlockingQueue make this > > code a little simpler? > > Hari Shreedharan wrote: > I don't see it becoming much simpler. I also prefer having the threading > code being more explicit. A blocking queue does not really help, since we are > lazily initializing the clients, so we need locking for that anyway.
Sorry I thought I had replaced this comment with the one below. > On Feb. 10, 2013, 9:11 p.m., Brock Noland wrote: > > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java, line > > 337 > > <https://reviews.apache.org/r/9284/diff/10/?file=257203#file257203line337> > > > > I feel like we could do this in a little simpler fashion with a > > Semaphore? Hari, Right now the checkout method has 9 lines of code doing non initialization work. Could this be done in 4-5 lines with a semaphore? I feel that is simpler and achieves the same result. semaphore.acquire(1); o = availableClients.poll(); if(o == null) { o = createNew(); checkedOutClients.add(o); } then on return availableClients.put(o); checkedOutClients.remove(o); semaphore.release(1); - Brock ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9284/#review16399 ----------------------------------------------------------- On Feb. 9, 2013, 2:28 a.m., Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9284/ > ----------------------------------------------------------- > > (Updated Feb. 9, 2013, 2:28 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 > >
