Do not reuse the Failed OpAddEntry object which lead bundle unloading timeout.

[codelipenghui]

Motivation

There are 2 ways to complete the OpAddEntry with exception, one is the bk client callback and another one is ManagedLedgerImple.clearPendingAddEntries.
But, if the OpAddEntry be completed more than once, we will get NPE:

java.lang.NullPointerException: null
        at org.apache.bookkeeper.mledger.impl.OpAddEntry.lambda$handleAddFailure$0(OpAddEntry.java:291) ~[org.apache.pulsar-managed-ledger-2.8.1.jar:2.8.1]
        at org.apache.bookkeeper.mledger.util.SafeRun$1.safeRun(SafeRun.java:32) ~[org.apache.pulsar-managed-ledger-2.8.1.jar:2.8.1]
        at org.apache.bookkeeper.common.util.SafeRunnable.run(SafeRunnable.java:36) [org.apache.bookkeeper-bookkeeper-common-4.14.2-2.jar:4.14.2-2]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [?:1.8.0_302]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [?:1.8.0_302]
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [io.netty-netty-common-4.1.68.Final.jar:4.1.68.Final]
        at java.lang.Thread.run(Thread.java:748) [?:1.8.0_302]

Another one:

java.lang.NullPointerException: null
        at org.apache.bookkeeper.mledger.impl.OpAddEntry.addComplete(OpAddEntry.java:153) ~[org.apache.pulsar-managed-ledger-2.8.1.jar:2.8.1]
        at org.apache.bookkeeper.client.AsyncCallback$AddCallback.addCompleteWithLatency(AsyncCallback.java:92) ~[org.apache.bookkeeper-bookkeeper-server-4.14.2-2.jar:4.14.2-2]
        at org.apache.bookkeeper.client.PendingAddOp.submitCallback(PendingAddOp.java:431) ~[org.apache.bookkeeper-bookkeeper-server-4.14.2-2.jar:4.14.2-2]
        at org.apache.bookkeeper.client.LedgerHandle.errorOutPendingAdds(LedgerHandle.java:1799) ~[org.apache.bookkeeper-bookkeeper-server-4.14.2-2.jar:4.14.2-2]
        at org.apache.bookkeeper.client.LedgerHandle$5.safeRun(LedgerHandle.java:574) ~[org.apache.bookkeeper-bookkeeper-server-4.14.2-2.jar:4.14.2-2]
        at org.apache.bookkeeper.common.util.SafeRunnable.run(SafeRunnable.java:36) [org.apache.bookkeeper-bookkeeper-common-4.14.2-2.jar:4.14.2-2]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [?:1.8.0_302]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [?:1.8.0_302]
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [io.netty-netty-common-4.1.68.Final.jar:4.1.68.Final]
        at java.lang.Thread.run(Thread.java:748) [?:1.8.0_302]

#12364 tries to fix the NPE by change the state of the OpAddEntry to CLOSED, but the OpAddEntry still will be recyled and be reused.
And when we get the add entry complete callback from the bk client, we will reach here:

pulsar/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java

Line 147 in 5dbb7d2

if (!STATE_UPDATER.compareAndSet(OpAddEntry.this, State.INITIATED, State.COMPLETED)) {

But it might to recycle the OpAddEntry already be reused by other entry add operation. It might lead to lost data for this case.

Modification

So the fix is do not recycle the OpAddEntry when call OpAddEntry.failed() which is introduced by #11737.

We should contain this fix in 2.8.2, we have encounter serious problem when unloading the bundles, the topic close will be blocked and never complete
because of LedgerHandle.errorOutPendingAdds() https://github.com/apache/bookkeeper/blob/87579b0a9f18833ee41fcae37582bb68606d68e7/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java#L574 get NPE but can't throw out and the ledger close callback will never complete.

Need to update docs?

• doc-required

  (If you need help on updating docs, create a doc issue)

• no-need-doc

  (Please explain why)

• doc

  (If this PR contains doc changes)

[hangc0276]

Nice Catch!

[eolivelli]

lgtm

is this problem present in 2.9.0 as well ?

[eolivelli]

@nicoloboschi PTAL

[codelipenghui]

is this problem present in 2.9.0 as well ?

@eolivelli Yes

[nicoloboschi]

It makes sense. If we don't recycle the object we can actually skip updating the state to CLOSED

[shoothzj]

@codelipenghui what's the relationship between this and #12396 . Does this mean we don't need it anymore？

[michaeljmarshall]

@codelipenghui - can you explain why you removed this line? Without it, when the addComplete callback is run, the method won't exit early because the state for the OpAddEntry will still be INITIATED.

[michaeljmarshall]

After thinking about this a little more, I think we'll need a more nuanced solution than just re-adding op.close(); on this line. I have to sign off for now. I'll follow up with more commentary tomorrow.

[codelipenghui]

@michaeljmarshall we will not reuse the failed OpAddEntry, so we don't need to update the state of the OpAddEntry here.

[michaeljmarshall]

@codelipenghui - okay. My concern was that if we call clearPendingEntries before the BK callback completes, we would want the state to be closed so that the callback doesn't run further.

[Jason918]

@codelipenghui what's the relationship between this and #12396 . Does this mean we don't need it anymore？

NO relationship. It's OpReadEntry in #12396.

[michaeljmarshall]

@codelipenghui - when you ran into this error, did you have AddEntryTimeoutSeconds configured to a non zero value?