> 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? > > Brock Noland wrote: > 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, I initially worked with this logic. But the reason I did not do this was that using a semaphore would mean that availableClients and checkedOutClients be thread-safe data structure, like a ConcurrentList or BlockingQueue or something. This would mean 3 lock/unlock cycles per checkOut and checkIn - one inside the semaphore code, one in each of the queue/sets. I was trying to avoid this. Since the critical sections are relatively small - the lock/unlock cost actually is more when we have 3 locks. The only real advantage I see from this approach is that the RpcClient can be created without holding the lock, but this happens relatively rarely in the life of the program, so I wouldn't worry too much about it. Agreed that the code becomes more concise, but I don't think we really need 3 locks to handle this especially in 3 consecutive lines of otherwise inexpensive code. - Hari ----------------------------------------------------------- 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 > >
