[broker] Timeout API calls in BrokerService #6489

addisonj · 2020-03-06T01:31:02Z

See #6416. This change ensures that all futures within BrokerService
have a guranteed timeout. As stated in #6416, we see cases where it
appears that loading or creating a topic fails to resolve the future for
unknown reasons. It appears that these futures may not be returning.

Motivation

This commit just adds a deadline for all the futures created in brokerService.

This seems like a sane change to make to ensure that these futures
finish, however, it still isn't understood under what conditions these
futures may not be returning, so this fix is mostly a workaround for
some underlying issues.

Verifying this change

This change is already covered by existing tests, such as the broker tests

(or)

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): n
The public API:n
The schema: n
The default values of configurations: n
The wire protocol:n
The rest endpoints: n
The admin cli options: n
Anything that affects deployment: n

Documentation

Does this pull request introduce a new feature? n
If yes, how is the feature documented? n/a
If a feature is not applicable for documentation, explain why?
If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

See apache#6416. This change ensures that all futures within BrokerService have a guranteed timeout. As stated in apache#6416, we see cases where it appears that loading or creating a topic fails to resolve the future for unknown reasons. It appears that these futures *may* not be returning. This seems like a sane change to make to ensure that these futures finish, however, it still isn't understood under what conditions these futures may not be returning, so this fix is mostly a workaround for some underlying issues

addisonj · 2020-03-06T02:19:00Z

/pulsarbot run-failure-checks

addisonj · 2020-03-06T03:02:59Z

/pulsarbot run-failure-checks

sijie · 2020-03-06T23:04:25Z

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

@@ -732,6 +733,20 @@ public void unloadNamespaceBundlesGracefully() {
        return future;
    }

+    private <T> CompletableFuture<T> futureWithDeadline(Long delay, TimeUnit unit, Exception exp) {


Can we move this into FutureUtil class?

See apache#6416. This change ensures that all futures within BrokerService have a guranteed timeout. As stated in apache#6416, we see cases where it appears that loading or creating a topic fails to resolve the future for unknown reasons. It appears that these futures *may* not be returning. This seems like a sane change to make to ensure that these futures finish, however, it still isn't understood under what conditions these futures may not be returning, so this fix is mostly a workaround for some underlying issues Co-authored-by: Addison Higham <ahigham@instructure.com> (cherry picked from commit 4a4cce9)

See #6416. This change ensures that all futures within BrokerService have a guranteed timeout. As stated in #6416, we see cases where it appears that loading or creating a topic fails to resolve the future for unknown reasons. It appears that these futures *may* not be returning. This seems like a sane change to make to ensure that these futures finish, however, it still isn't understood under what conditions these futures may not be returning, so this fix is mostly a workaround for some underlying issues Co-authored-by: Addison Higham <ahigham@instructure.com> (cherry picked from commit 4a4cce9)

In apache#6489, a timeout was introduced to make sure calls into the BrokerService finish or error out. However, this timeout is too low by default when loading topics that have many replicated clusters. Loading replicated topics is quite an expensive operation, involve global ZK lookups and the start of many sub-processes. While we would hope it finishes in 60 seconds we want to safe. Long term, it may make sense to break out this operation into more steps where each step can have it's own timeout

In #6489, a timeout was introduced to make sure calls into the BrokerService finish or error out. However, this timeout is too low by default when loading topics that have many replicated clusters. Loading replicated topics is quite an expensive operation, involve global ZK lookups and the start of many sub-processes. While we would hope it finishes in 60 seconds we want to safe. Long term, it may make sense to break out this operation into more steps where each step can have it's own timeout Co-authored-by: Addison Higham <ahigham@instructure.com>

In apache#6489, a timeout was introduced to make sure calls into the BrokerService finish or error out. However, this timeout is too low by default when loading topics that have many replicated clusters. Loading replicated topics is quite an expensive operation, involve global ZK lookups and the start of many sub-processes. While we would hope it finishes in 60 seconds we want to safe. Long term, it may make sense to break out this operation into more steps where each step can have it's own timeout

In #6489, a timeout was introduced to make sure calls into the BrokerService finish or error out. However, this timeout is too low by default when loading topics that have many replicated clusters. Loading replicated topics is quite an expensive operation, involve global ZK lookups and the start of many sub-processes. While we would hope it finishes in 60 seconds we want to safe. Long term, it may make sense to break out this operation into more steps where each step can have it's own timeout Co-authored-by: Addison Higham <ahigham@instructure.com>(cherry picked from commit 6854b00)

See apache#6416. This change ensures that all futures within BrokerService have a guranteed timeout. As stated in apache#6416, we see cases where it appears that loading or creating a topic fails to resolve the future for unknown reasons. It appears that these futures *may* not be returning. This seems like a sane change to make to ensure that these futures finish, however, it still isn't understood under what conditions these futures may not be returning, so this fix is mostly a workaround for some underlying issues Co-authored-by: Addison Higham <ahigham@instructure.com>(cherry picked from commit 4a4cce9)

In apache#6489, a timeout was introduced to make sure calls into the BrokerService finish or error out. However, this timeout is too low by default when loading topics that have many replicated clusters. Loading replicated topics is quite an expensive operation, involve global ZK lookups and the start of many sub-processes. While we would hope it finishes in 60 seconds we want to safe. Long term, it may make sense to break out this operation into more steps where each step can have it's own timeout Co-authored-by: Addison Higham <ahigham@instructure.com>

In apache#6489, a timeout was introduced to make sure calls into the BrokerService finish or error out. However, this timeout is too low by default when loading topics that have many replicated clusters. Loading replicated topics is quite an expensive operation, involve global ZK lookups and the start of many sub-processes. While we would hope it finishes in 60 seconds we want to safe. Long term, it may make sense to break out this operation into more steps where each step can have it's own timeout

See apache#6416. This change ensures that all futures within BrokerService have a guranteed timeout. As stated in apache#6416, we see cases where it appears that loading or creating a topic fails to resolve the future for unknown reasons. It appears that these futures *may* not be returning. This seems like a sane change to make to ensure that these futures finish, however, it still isn't understood under what conditions these futures may not be returning, so this fix is mostly a workaround for some underlying issues Co-authored-by: Addison Higham <ahigham@instructure.com>

In apache#6489, a timeout was introduced to make sure calls into the BrokerService finish or error out. However, this timeout is too low by default when loading topics that have many replicated clusters. Loading replicated topics is quite an expensive operation, involve global ZK lookups and the start of many sub-processes. While we would hope it finishes in 60 seconds we want to safe. Long term, it may make sense to break out this operation into more steps where each step can have it's own timeout Co-authored-by: Addison Higham <ahigham@instructure.com>

sijie reviewed Mar 6, 2020

View reviewed changes

sijie assigned addisonj Mar 6, 2020

sijie added area/broker release/2.5.1 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages labels Mar 6, 2020

sijie added this to the 2.6.0 milestone Mar 6, 2020

sijie approved these changes Mar 18, 2020

View reviewed changes

sijie merged commit 4a4cce9 into apache:master Mar 18, 2020

addisonj mentioned this pull request Apr 17, 2020

[broker] Increase timeout for loading topics #6750

Merged

codelipenghui mentioned this pull request May 5, 2020

Producer cannot connect after Unloading bundle #6872

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[broker] Timeout API calls in BrokerService #6489

[broker] Timeout API calls in BrokerService #6489

addisonj commented Mar 6, 2020

addisonj commented Mar 6, 2020

addisonj commented Mar 6, 2020

sijie Mar 6, 2020

[broker] Timeout API calls in BrokerService #6489

[broker] Timeout API calls in BrokerService #6489

Conversation

addisonj commented Mar 6, 2020

Motivation

Verifying this change

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

Documentation

addisonj commented Mar 6, 2020

addisonj commented Mar 6, 2020

sijie Mar 6, 2020

Choose a reason for hiding this comment