Shutdown Broker gracefully, but forcefully after brokerShutdownTimeoutMs #10199
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This PRs continues the work started in PR #9308 . |
michaeljmarshall
requested changes
Apr 13, 2021
...common/src/main/java/org/apache/pulsar/common/util/CompletableFutureCancellationHandler.java
Show resolved
Hide resolved
lhotari
force-pushed
the
lh-completely-shutdown-broker
branch
from
dfc6747
to
32fa7d1
Compare
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/MessagingServiceShutdownHook.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
Outdated
Show resolved
Hide resolved
lhotari
force-pushed
the
lh-completely-shutdown-broker
branch
2 times, most recently
from
405c093
to
e1febb1
Compare
|
/pulsarbot run-failure-checks |
1 similar comment
|
/pulsarbot run-failure-checks |
lhotari
force-pushed
the
lh-completely-shutdown-broker
branch
from
e1febb1
to
79e800a
Compare
codelipenghui
approved these changes
Apr 15, 2021
lhotari
force-pushed
the
lh-completely-shutdown-broker
branch
from
79e800a
to
4e7488a
Compare
|
@merlimat to review |
|
/pulsarbot run-failure-checks |
merlimat
reviewed
Apr 15, 2021
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/pulsar/broker/service/GracefulExecutorServicesShutdownHandler.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/pulsar/broker/service/GracefulExecutorServicesShutdownHandler.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/pulsar/broker/service/GracefulExecutorServicesShutdownHandler.java
Outdated
Show resolved
Hide resolved
@merlimat Thank you for your review and good points about await termination. |
- make cancelAction eligible for GC after the future completes (or gets cancelled)
- shutdown forcefully when closing times out
- handle graceful / forcefully shutdown also for PulsarService executors
lhotari
force-pushed
the
lh-completely-shutdown-broker
branch
from
532e51c
to
e4ad678
Compare
|
/pulsarbot run-failure-checks |
merlimat
approved these changes
Apr 16, 2021
jiazhai
pushed a commit
to streamnative/kop
that referenced
this pull request
Jul 29, 2021
### Motivation The KoP CI tests take much more time than CI tests of branch-2.7.2. The main reason is the `cleanup()` phase takes a long time, each time a test is cleaned up, it will take over 10 seconds to complete. This behavior was introduced from apache/pulsar#10199, which made broker shutdown gracefully by default but it would take longer to shutdown. The other reason is caused by rebalance time. According to my observes, when a Kafka consumer subscribes a topic in KoP, it will take at least 3 seconds. Finally I found it's caused by the GroupInitialRebalanceDelayMs config, which has the same semantics with Kafka's [group.initial.rebalance.delay.ms](https://kafka.apache.org/documentation/#brokerconfigs_group.initial.rebalance.delay.ms). It makes Kafka server wait longer for `JOIN_GROUP` request for more consumers to join so that the rebalance count can reduce. However, it should be set zero in tests. After fixing these problems, sometimes the following error may happen and cause flakiness. ``` TwoPhaseCompactor$MockitoMock$1432102834 cannot be returned by getConfiguration() getConfiguration() should return ServiceConfiguration *** If you're unsure why you're getting above error read on. Due to the nature of the syntax above problem might occur because: 1. This exception *might* occur in wrongly written multi-threaded tests. Please refer to Mockito FAQ on limitations of concurrency testing. 2. A spy is stubbed using when(spy.foo()).then() syntax. It is safer to stub spies - - with doReturn|Throw() family of methods. More in javadocs for Mockito.spy() method. ``` It's because `PulsarService#newCompactor` is not mocked well, see apache/pulsar#7102 for detail. ### Modifications - Configure `GroupInitialRebalanceDelayMs` and `BrokerShutdownTimeoutMs` for each mocked `BrokerService`. - Fix the flakiness caused by mocking compactor. After the changes, the tests time has a significant improvement. For example, `GroupCoordinatorTest` takes only 3 minutes now but it could take 9 minutes before. Because the `cleanup()` method is marked as `@AfterMethod` and would be called each time a single test finished. Another example is that `BasicEndToEndKafkaTest` takes only 37 seconds now but it could take 56 seconds before. The `cleanup()` is marked as `@AfterClass` and only happens once, but many consumers will be created during the whole tests and each time a subscribe call can take 3 seconds. * Speed up tests and fix flakiness * Speed up tests that creat their own configs * Ignore golang-sarama tests
BewareMyPower
added a commit
to streamnative/kop
that referenced
this pull request
Aug 5, 2021
### Motivation The KoP CI tests take much more time than CI tests of branch-2.7.2. The main reason is the `cleanup()` phase takes a long time, each time a test is cleaned up, it will take over 10 seconds to complete. This behavior was introduced from apache/pulsar#10199, which made broker shutdown gracefully by default but it would take longer to shutdown. The other reason is caused by rebalance time. According to my observes, when a Kafka consumer subscribes a topic in KoP, it will take at least 3 seconds. Finally I found it's caused by the GroupInitialRebalanceDelayMs config, which has the same semantics with Kafka's [group.initial.rebalance.delay.ms](https://kafka.apache.org/documentation/#brokerconfigs_group.initial.rebalance.delay.ms). It makes Kafka server wait longer for `JOIN_GROUP` request for more consumers to join so that the rebalance count can reduce. However, it should be set zero in tests. After fixing these problems, sometimes the following error may happen and cause flakiness. ``` TwoPhaseCompactor$MockitoMock$1432102834 cannot be returned by getConfiguration() getConfiguration() should return ServiceConfiguration *** If you're unsure why you're getting above error read on. Due to the nature of the syntax above problem might occur because: 1. This exception *might* occur in wrongly written multi-threaded tests. Please refer to Mockito FAQ on limitations of concurrency testing. 2. A spy is stubbed using when(spy.foo()).then() syntax. It is safer to stub spies - - with doReturn|Throw() family of methods. More in javadocs for Mockito.spy() method. ``` It's because `PulsarService#newCompactor` is not mocked well, see apache/pulsar#7102 for detail. ### Modifications - Configure `GroupInitialRebalanceDelayMs` and `BrokerShutdownTimeoutMs` for each mocked `BrokerService`. - Fix the flakiness caused by mocking compactor. After the changes, the tests time has a significant improvement. For example, `GroupCoordinatorTest` takes only 3 minutes now but it could take 9 minutes before. Because the `cleanup()` method is marked as `@AfterMethod` and would be called each time a single test finished. Another example is that `BasicEndToEndKafkaTest` takes only 37 seconds now but it could take 56 seconds before. The `cleanup()` is marked as `@AfterClass` and only happens once, but many consumers will be created during the whole tests and each time a subscribe call can take 3 seconds. * Speed up tests and fix flakiness * Speed up tests that creat their own configs * Ignore golang-sarama tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Broker shutdown isn't currently graceful although the intention for it is to be graceful.
Another problem in tests is that broker shutdown doesn't shutdown synchronously.
These 2 problems are addressed in this PR.
The changes 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.
Modifications