Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async the method onManagedLedgerLastLedgerInitialize for ManagedLedgerInterceptor #10706

Merged
merged 2 commits into from
May 26, 2021

Conversation

codelipenghui
Copy link
Contributor

@codelipenghui codelipenghui commented May 26, 2021

Fixes #10703

Motivation

Async the method onManagedLedgerLastLedgerInitialize for ManagedLedgerInterceptor. The root cause for issue #10703 is the ManagedLedger Interceptor is making a block read call from within a thread used for async operations. The interceptor shouldn’t be making any blocking calls. Thanks for @merlimat's help.

Here is the detailed stacktrace:

"BookKeeperClientWorker-OrderedExecutor-0-0" #67 prio=5 os_prio=0 cpu=50.69ms elapsed=14871.45s allocated=4671K defined_classes=63 tid=0x00007f8cee18e560 nid=0x9b01 waiting on condition  [0x00007f8b8fbfa000]
   java.lang.Thread.State: WAITING (parking)
 at jdk.internal.misc.Unsafe.park(java.base@11.0.11/Native Method)
 - parking to wait for  <0x0000000602818fc8> (a java.util.concurrent.CompletableFuture$Signaller)
 at java.util.concurrent.locks.LockSupport.park(java.base@11.0.11/LockSupport.java:194)
 at java.util.concurrent.CompletableFuture$Signaller.block(java.base@11.0.11/CompletableFuture.java:1796)
 at java.util.concurrent.ForkJoinPool.managedBlock(java.base@11.0.11/ForkJoinPool.java:3128)
 at java.util.concurrent.CompletableFuture.waitingGet(java.base@11.0.11/CompletableFuture.java:1823)
 at java.util.concurrent.CompletableFuture.get(java.base@11.0.11/CompletableFuture.java:1998)
 at org.apache.bookkeeper.common.concurrent.FutureUtils.result(FutureUtils.java:72)
 at org.apache.bookkeeper.client.api.ReadHandle.read(ReadHandle.java:58)
 at org.apache.pulsar.broker.intercept.ManagedLedgerInterceptorImpl.onManagedLedgerLastLedgerInitialize(ManagedLedgerInterceptorImpl.java:89)
 at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl$1.lambda$null$0(ManagedLedgerImpl.java:358)
 at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl$1$$Lambda$800/0x00000008406bb040.run(Unknown Source)
 at org.apache.bookkeeper.mledger.util.SafeRun$1.safeRun(SafeRun.java:32)
 at org.apache.bookkeeper.common.util.SafeRunnable.run(SafeRunnable.java:36)
 at java.util.concurrent.ThreadPoolExecutor.runWorker(java.base@11.0.11/ThreadPoolExecutor.java:1128)
 at java.util.concurrent.ThreadPoolExecutor$Worker.run(java.base@11.0.11/ThreadPoolExecutor.java:628)
 at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
 at java.lang.Thread.run(java.base@11.0.11/Thread.java:829)

   Locked ownable synchronizers:
 - <0x0000000601a92180> (a java.util.concurrent.ThreadPoolExecutor$Worker)

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

  • Does this pull request introduce a new feature? (no)

@codelipenghui codelipenghui self-assigned this May 26, 2021
@codelipenghui codelipenghui added this to the 2.8.0 milestone May 26, 2021
@codelipenghui codelipenghui added release/blocker Indicate the PR or issue that should block the release until it gets resolved type/bug The PR fixed a bug or issue reported a bug labels May 26, 2021
if (ledgerEntry != null) {
for (BrokerEntryMetadataInterceptor interceptor : brokerEntryMetadataInterceptors) {
if (interceptor instanceof AppendIndexMetadataInterceptor) {
BrokerEntryMetadata brokerEntryMetadata =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably just do the parsing once

Comment on lines 359 to 365
.whenComplete((__, ex) -> {
if (ex != null) {
callback.initializeFailed(new ManagedLedgerInterceptException(ex));
} else {
initializeBookKeeper(callback);
}
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general it's preferable to do:

Suggested change
.whenComplete((__, ex) -> {
if (ex != null) {
callback.initializeFailed(new ManagedLedgerInterceptException(ex));
} else {
initializeBookKeeper(callback);
}
});
.thenRun(() -> initializeBookKeeper(callback))
.exceptionally(ex -> {
callback.initializeFailed(new ManagedLedgerInterceptException(ex));
return null;
});

The reason is that if there is any unexpected exception thrown when calling initializeBookKeeper(), the exceptionally part will be invoked.

Copy link
Contributor

@hangc0276 hangc0276 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good Job!

@merlimat merlimat merged commit f355272 into apache:master May 26, 2021
@codelipenghui codelipenghui deleted the penghui/fix-10703 branch May 26, 2021 07:21
BewareMyPower pushed a commit to streamnative/kop that referenced this pull request May 31, 2021
### Modification
1. upgrade pulsar dependency to include the following bug fix
    - apache/pulsar#10706 : fix load __consumer_offset topic timeout on bundle onLoad stage
    - apache/pulsar#10341 :  fix create reader failed on bundle onLoad stage
yangl pushed a commit to yangl/pulsar that referenced this pull request Jun 23, 2021
…rInterceptor (apache#10706)

* Async the method onManagedLedgerLastLedgerInitialize for ManagedLedgerInterceptor

* Addressed comment.
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
…rInterceptor (apache#10706)

* Async the method onManagedLedgerLastLedgerInitialize for ManagedLedgerInterceptor

* Addressed comment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/blocker Indicate the PR or issue that should block the release until it gets resolved type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Managed ledger can't been initialized
5 participants