thanks Flavio for reviewing.
> but not the one in doRecoveryRead. Why do we need to give it to a worker thread in this case?
doRecoveryRead() will be executed in #addComplete, which is also in the callback of bookkeeper. so it should do same as what it did in #readComplete.
> It is not performance critical in this case, but it sounds like a good ideal in general to have LOG.debug statements wrapped with isDebugEnabled() (LedgerRecoveryOp:86). You may have simply missed this one.
if we use '+' to concatenate string it would introduce the performance issue. but we don't concatenate the string in debug message, so it would not introduce the performance issue. besides that, I remembered that because we use slf4j, Ivan suggested to use such kind of pattern to log debug message.
> Is this change gratuitous or really necessary:
actually I used this method in hedwig BookKeeperTestBase class, which extends existed Bookie class to simulate response delay for #readEntry. It helps testing the deadlock in bookkeeper persistence manager.
> testRecoveryDeadlockWithLimitedPermits() has no assertion or fail clause. What is it testing?
if we don't apply the patch, the test case will hang due to deadlock. so I am not sure how to add assertion and fail clause. what is your opinion?
> I'm not entirely sure why we need this method:
the method is used in TestDeadLock#getServerConfiguration. it used to load a bookkeeper client configuration object. so the hub server could use bookkeeper client settings we provided.
> In TestDeadlock, if I understand the test correctly, consumeQueue.take() is supposed to hang due to the bug of this jira. Consequently, we have to wait until junit times out the test? I was wondering if there is a way of avoiding the time out.
yes. we have to wait until timeout if we don't apply this patch. I have no perfect solution to test such hang due to deadlock.
> typos & comments
yeah. thanks for fixing them. I would update them to new patch.