> 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
>
> Hari Shreedharan wrote:
> 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.
Also, this code is actually the standard -
enter monitor:
while(condition) wait()
..
..
enter monitor:
signal()
idiom. I don't really think it is less readable either ;)
- 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
>
>
