zhaohaidao commented on code in PR #3620:
URL: https://github.com/apache/bookkeeper/pull/3620#discussion_r1016152564
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java:
##########
@@ -266,6 +266,10 @@ public void run() {
// We are about to send. Check if we need to make an ensemble change
// because of delayed write errors
lh.maybeHandleDelayedWriteBookieFailure();
+ // This op maybe recycled during bookie failures
+ if (maybeRecycled()) {
Review Comment:
> So maybeHandleDelayedWriteBookieFailure triggers something that recycles
this pending add op (I haven't looked that closely)?
>
> Then the fix masks the problem. One side of the problem is that lh becomes
null / results in NPE later. Another side of the problem is that pending add
opp can be recycled and immediately reused by another add.
- One side of the problem is that lh becomes null / results in NPE later:
If I understand correctly, only maybeHandleDelayedWriteBookieFailure has a
chance to recycle this operation causing lh to become null which leads to this
npe issue. Although this behavior is not as described in the followed comments
about recycle condition, I think it is reasonable.
```
private void maybeRecycle() {
/**
* We have opportunity to recycle two objects here.
* PendingAddOp#toSend and LedgerHandle#pendingAddOp
*
* A. LedgerHandle#pendingAddOp: This can be released after all 3
conditions are met.
* - After the callback is run
* - After safeRun finished by the executor
* - Write responses are returned from all bookies
* as BookieClient Holds a reference from the point the
addEntry requests are sent.
*
* B. ByteBuf can be released after 2 of the conditions are met.
* - After the callback is run as we will not retry the write
after callback
* - After safeRun finished by the executor
* BookieClient takes and releases on this buffer immediately after
sending the data.
*
* The object can only be recycled after the above conditions are met
* otherwise we could end up recycling twice and all
* joy that goes along with that.
*/
if (hasRun && callbackTriggered) {
ReferenceCountUtil.release(toSend);
toSend = null;
}
// only recycle a pending add op after it has been run.
if (hasRun && toSend == null && pendingWriteRequests == 0) {
recyclePendAddOpObject();
}
}
```
- Another side of the problem is that pending add opp can be recycled and
immediately reused by another add.: I don't understand this very well, can you
explain in more detail?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
