Wait for the async broker port listener close operations to complete at shutdown #9308

lhotari · 2021-01-25T10:49:50Z

Motivation

The main motivation for making this change to shut down port listeners synchronously in BrokerService.close is to reduce test flakiness.

While investigating the flaky test MessageIdTest, these type of exceptions were seen in logs:

Caused by: java.util.concurrent.RejectedExecutionException: Task org.apache.pulsar.metadata.impl.AbstractMetadataStore$$Lambda$669/1529307342@2f790c2a rejected from java.util.concurrent.ThreadPoolExecutor@ad9a9ac[Terminated, pool size = 0, active threads = 0, queued tasks = 0, completed tasks = 9]
        at java.util.concurrent.ThreadPoolExecutor$AbortPolicy.rejectedExecution(ThreadPoolExecutor.java:2063) ~[?:1.8.0_275]
        at java.util.concurrent.ThreadPoolExecutor.reject(ThreadPoolExecutor.java:830) ~[?:1.8.0_275]
        at java.util.concurrent.ThreadPoolExecutor.execute(ThreadPoolExecutor.java:1379) ~[?:1.8.0_275]
        at java.util.concurrent.Executors$DelegatedExecutorService.execute(Executors.java:668) ~[?:1.8.0_275]
        at org.apache.pulsar.metadata.impl.AbstractMetadataStore.receivedNotification(AbstractMetadataStore.java:128) ~[pulsar-metadata-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
        at org.apache.pulsar.metadata.impl.ZKMetadataStore.process(ZKMetadataStore.java:320) ~[pulsar-metadata-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
        at org.apache.zookeeper.MockZooKeeper.lambda$setData$16(MockZooKeeper.java:728) ~[testmocks-2.8.0-SNAPSHOT.jar:3.5.7]
        at com.google.common.util.concurrent.MoreExecutors$DirectExecutorService.execute(MoreExecutors.java:321) ~[guava-30.1-jre.jar:?]
        at org.apache.zookeeper.MockZooKeeper.setData(MockZooKeeper.java:683) ~[testmocks-2.8.0-SNAPSHOT.jar:3.5.7]
        at org.apache.pulsar.metadata.impl.ZKMetadataStore.put(ZKMetadataStore.java:207) ~[pulsar-metadata-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
        at org.apache.bookkeeper.mledger.impl.MetaStoreImpl.asyncUpdateLedgerIds(MetaStoreImpl.java:106) ~[managed-ledger-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
        at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl.lambda$null$1(ManagedLedgerImpl.java:472) ~[managed-ledger-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
        at org.apache.bookkeeper.mledger.util.SafeRun$1.safeRun(SafeRun.java:32) ~[managed-ledger-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
        ... 6 more

It seems that this problem could occur when the test code is able to access the broker instance that is already terminated.

Modifications

To understand the modifications:
The original solution was to wait up to 10 seconds until the port listeners were closed.
A change was requested to add a new setting for this. After adding this, another change was requested to combine these operations in a way that the existing brokerShutdownTimeoutMs setting would also apply to the closing of the port listeners.
This solution for this adds BrokerService.closeAsync and PulsarService.closeAsync methods so that it's possible to combine the closing of the ports to the close operation that uses brokerShutdownTimeoutMs setting.

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

eolivelli

LGTM

lhotari · 2021-01-26T19:25:35Z

/pulsarbot run-failure-checks

lhotari · 2021-01-27T04:46:55Z

/pulsarbot run-failure-checks

lhotari · 2021-01-27T08:44:14Z

/pulsarbot run-failure-checks

lhotari · 2021-01-27T12:11:03Z

/pulsarbot run-failure-checks

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

lhotari · 2021-03-22T05:51:33Z

@merlimat I have revisited the solution by adding closeAsync methods so that it's possible to wait for shutdown using the existing brokerShutdownTimeoutMs setting. PTAL

pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

lhotari · 2021-03-22T08:28:31Z

/pulsarbot run-failure-checks

pulsar-broker/src/main/java/org/apache/pulsar/broker/MessagingServiceShutdownHook.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

lhotari · 2021-03-25T08:45:09Z

@eolivelli I have made the change to closeAsync so that it doesn't throw checked exceptions.

I took another look at MessagingServiceShutdownHook logic and unfortunately I didn't find a way to simplify the logic. There the intention is to run the shutdown in a background thread so that the actual shutdown can continue if the shutdown takes longer than the timeout. Although the code might look bad, there isn't a real alternative. Catching Exceptions is necessary to catch also possible runtime exceptions that could happen at shutdown. You can try to suggest a better way, I tried and simply didn't find one. :)

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

lhotari · 2021-03-25T11:07:44Z

/pulsarbot run-failure-checks

lhotari · 2021-03-25T12:38:30Z

/pulsarbot run-failure-checks

lhotari · 2021-03-25T15:19:22Z

/pulsarbot run-failure-checks

lhotari · 2021-03-25T17:05:58Z

/pulsarbot run-failure-checks

lhotari · 2021-03-25T21:01:59Z

/pulsarbot run-failure-checks

- add BrokerService.closeAsync and PulsarService.closeAsync so that shutdown can handle asynchronous closing operations

- address review comment

eolivelli

looks great to me

lhotari · 2021-03-26T09:34:00Z

/pulsarbot run-failure-checks

lhotari · 2021-03-26T20:34:33Z

@merlimat Please review this PR since it has changed since you approved it.

merlimat

Looks good. Just a couple of comments

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

Catching RuntimeExceptions isn't sufficient since checked exceptions can be thrown in the JVM with solutions like "sneaky throws" This reverts commit 01b5f6f.

…usage

eolivelli

Still good for me

lhotari · 2021-03-27T13:54:39Z

/pulsarbot run-failure-checks

merlimat

👍

lhotari · 2021-04-15T12:57:15Z

@sijie @merlimat I have continued the work started in this PR in another PR . Please review #10199 since it will also help improve CI stability when the asynchronous tasks of broker shutdown can be controlled in tests. This prevents problems which are caused by too many brokers being active at the same time. This could currently happen when previous brokers are asynchronously shutting down.

lhotari mentioned this pull request Jan 25, 2021

Fix flaky MessageIdTest and introduce some testing improvements #9286

Merged

merlimat approved these changes Jan 25, 2021

View reviewed changes

sijie requested changes Jan 25, 2021

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java Outdated Show resolved Hide resolved

sijie assigned lhotari Jan 25, 2021

sijie added area/broker area/config labels Jan 25, 2021

sijie added this to the 2.8.0 milestone Jan 25, 2021

lhotari requested a review from sijie January 26, 2021 13:41

eolivelli approved these changes Jan 26, 2021

View reviewed changes

merlimat reviewed Jan 27, 2021

View reviewed changes

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java Outdated Show resolved Hide resolved

lhotari force-pushed the lh-fix-broker-shutdown branch from f43e286 to 0714b29 Compare March 22, 2021 05:49

lhotari changed the title ~~Wait for broker port listeners to shutdown in BrokerService.close~~ Wait for the async broker port listener close operations to complete at shutdown Mar 22, 2021

lhotari requested review from eolivelli and merlimat March 22, 2021 06:00

eolivelli requested changes Mar 22, 2021

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java Outdated Show resolved Hide resolved

eolivelli reviewed Mar 22, 2021

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/MessagingServiceShutdownHook.java Show resolved Hide resolved

eolivelli reviewed Mar 22, 2021

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java Outdated Show resolved Hide resolved

lhotari force-pushed the lh-fix-broker-shutdown branch from 0714b29 to 2faceb6 Compare March 25, 2021 08:38

eolivelli reviewed Mar 25, 2021

View reviewed changes

lhotari added 2 commits March 26, 2021 08:42

Wait for closing of ports at shutdown

9e4de20

- add BrokerService.closeAsync and PulsarService.closeAsync so that shutdown can handle asynchronous closing operations

Don't throw checked exceptions from methods returning CompletableFuture

81b5945

- address review comment

lhotari force-pushed the lh-fix-broker-shutdown branch from 2faceb6 to 81b5945 Compare March 26, 2021 06:56

lhotari added 2 commits March 26, 2021 09:03

Set interrupt status when catching InterruptedException

cace052

Catch RuntimeException instead of Exception

01b5f6f

lhotari requested a review from eolivelli March 26, 2021 07:12

eolivelli approved these changes Mar 26, 2021

View reviewed changes

codelipenghui approved these changes Mar 26, 2021

View reviewed changes

sijie approved these changes Mar 26, 2021

View reviewed changes

merlimat reviewed Mar 26, 2021

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java Outdated Show resolved Hide resolved

pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java Outdated Show resolved Hide resolved

lhotari added 3 commits March 27, 2021 14:12

Revert "Catch RuntimeException instead of Exception"

dedea0b

Catching RuntimeExceptions isn't sufficient since checked exceptions can be thrown in the JVM with solutions like "sneaky throws" This reverts commit 01b5f6f.

Address review comments about exception handling and AtomicReference …

7f2eca9

…usage

Fix checkstyle

309ebc9

eolivelli approved these changes Mar 27, 2021

View reviewed changes

merlimat approved these changes Mar 27, 2021

View reviewed changes

merlimat merged commit a9a2861 into apache:master Mar 27, 2021

lhotari mentioned this pull request Apr 12, 2021

Shutdown Broker gracefully, but forcefully after brokerShutdownTimeoutMs #10199

Merged

Wait for the async broker port listener close operations to complete at shutdown #9308

Wait for the async broker port listener close operations to complete at shutdown #9308

Uh oh!

Conversation

lhotari commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Uh oh!

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

lhotari commented Jan 26, 2021

Uh oh!

lhotari commented Jan 27, 2021

Uh oh!

lhotari commented Jan 27, 2021

Uh oh!

lhotari commented Jan 27, 2021

Uh oh!

Uh oh!

lhotari commented Mar 22, 2021

Uh oh!

Uh oh!

lhotari commented Mar 22, 2021

Uh oh!

Uh oh!

Uh oh!

lhotari commented Mar 25, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lhotari commented Mar 25, 2021

Uh oh!

lhotari commented Mar 25, 2021

Uh oh!

lhotari commented Mar 25, 2021

Uh oh!

lhotari commented Mar 25, 2021

Uh oh!

lhotari commented Mar 25, 2021

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

lhotari commented Mar 26, 2021

Uh oh!

lhotari commented Mar 26, 2021

Uh oh!

merlimat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

lhotari commented Mar 27, 2021

Uh oh!

merlimat left a comment

Choose a reason for hiding this comment

Uh oh!

lhotari commented Apr 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lhotari commented Jan 25, 2021 •

edited

Loading