> 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); > > Hari Shreedharan wrote: > 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. > > Brock Noland wrote: > Hi, > > I feel this is pre-mature optimization. In the mean time each thread will > be doing IO which is tremendously slow in comparison I don't think the faster > but less readable code will buy us much. With that said, I haven't proved > that and I don't feel strongly enough on the manner to hold up the commit. I > have just committed this. > > Brock
I agree that slow I/O threads can make things slow, but I don't still think it makes sense to use 3 blocking data structures when we could actually get away with none. Locks are expensive - even if they are not I/O level expensive, we are still hitting performance, when we don't really need to. - 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 > >
