Avoid acquiring closeLock.readLock() on every add/read operation #1292
Conversation
|
I am not sure that is right. If I remembered correctly, the lock is prevent client try to obtain a connect when closing. I think the reason you don't want contention in this level, because it is shared across all pcbc tools. you might consider pushing one level or two levels down. However if my understanding is wrong, you might also want to remove the read write lock variable, since we don't really need a rw lock after your change. |
Yes, though I think we should be able to do :
This should not require the RW lock
There is still the check when creating the PCBC pool. So it will check for it before creating a new connection, but not each time when using an existing connection. |
Is that in current PR? |
|
retest this please |
| } | ||
| pcbc.readLac(ledgerId, cb, ctx); | ||
| return; |
There was a problem hiding this comment.
the return is actually to avoid the pcbc.readLac(ledgerId, cb, ctx); but it would be clearer with an else block anyway.
| } else { | ||
| pcbc.writeLac(ledgerId, masterKey, lac, toSend, cb, ctx); | ||
| toSend.retain(); | ||
| client.obtain(new GenericCallback<PerChannelBookieClient>() { |
There was a problem hiding this comment.
nit: these can become lambdas
There was a problem hiding this comment.
make sense, I didn't do it initially since I just removed try/finally and de-indented, but since it anyway looks like these lines are changed.. I might as well convert them.
merlimat
force-pushed
the
bookie-client-rw-lock
branch
from
fbf3027
to
2104a3a
Compare
|
Rebased and addressed comments |
In the
BookieClient, we are always acquiring a readlock when grabbing a connection to use for sending a write/read request.The lock is the
closeLockand it's only acquired in "write" mode when theBookKeeperinstance is closed.The problem with the read-lock is that it introduces contention between the threads that are acquiring it (even if all of them in read mode). Multiple threads can be be in read mode in the critical section, though they have contention when they're entering/exiting the section.
Additionally, the Java implementation of read/write lock is creating and destroying a lot of objects when that contention happens.
My understanding of the code is that we don't need to acquire the read lock in that point. The reason is that, we are already acquiring the lock in the
lookupClient()method, although only if the pool is null. Additionally, whenBookkeeper.close()is invoked all PCBC will be set to closed as well, so it will not be possibile to create a new connection.All the line changes in the patch are just removing the readLock acquire and try/finally, and reducing the indentation level.