[Broker] Fix call sync method in async rest api for internalExpireMessagesByTimestamp #13880

liudezhi2098 · 2022-01-21T08:27:46Z

Motivation

Avoid call sync method in async rest API for PersistentTopicsBase#internalExpireMessagesByTimestamp.

Modifications

Use async instead of sync method.

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

Need to update docs?

no-need-doc

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

…is async method.

liudezhi2098 · 2022-01-22T06:33:42Z

/pulsarbot run-failure-checks

liudezhi2098 · 2022-01-23T00:56:10Z

/pulsarbot run-failure-checks

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

…o async

liudezhi2098 · 2022-01-24T12:18:25Z

/pulsarbot run-failure-checks

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

mattisonchao · 2022-01-26T10:08:44Z

Hi, @liudezhi2098

I think we need to refactor this method to make it more clear. some suggestions :

It is recommended to use "ThenCompose" to compose asynchronous methods and use the outermost "exceptionally" to handle exceptions. Because a lot of catch and exceptionally look like callback hell and hard to read.
Checked the "catch" logic, it seems that there are places where "catch" is not needed.

E.g.

Bad version:

   CompletableFuture.completedFuture(null)
                .thenAccept(__ -> {
                    CompletableFuture.completedFuture(null)
                            .thenAccept(unused2 -> {
                                CompletableFuture.completedFuture(null)
                                        .thenAccept(unused3 -> {

                                        }).exceptionally(ex -> {
                                            // handle exception
                                            return null;
                                        });
                            }).exceptionally(ex -> {
                                //handle exception
                                return null;
                            });
                }).exceptionally(ex -> {
                    //handle exception
                    return null;
                });

Good version:

        CompletableFuture.completedFuture(null)
                .thenCompose(__ -> CompletableFuture.completedFuture(null)
                        .thenCompose(unused2 -> CompletableFuture.completedFuture(null)
                                .thenCompose(unused3 -> {
                                    // do somethings
                                    return null;
                                })
                        )
                ).exceptionally(ex -> {
                    //handle exception
                    return null;
                });

liudezhi2098 · 2022-01-26T23:58:24Z

/pulsarbot run-failure-checks

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

…-master handle conflict

…reMessagesByTimestamp handle conflict

liudezhi2098 · 2022-01-27T07:40:49Z

/pulsarbot run-failure-checks

### Motivation This issue was triggered by #13880, it doesn't handle the NPE in [here](https://github.com/apache/pulsar/pull/13880/files#diff-66aeb65a64cbe7c541f013ae807c5bcbeab567bef77706c7ff2e0cbfe0d77eb1R3502), we can check the [diff code](https://github.com/apache/pulsar/pull/13880/files#diff-66aeb65a64cbe7c541f013ae807c5bcbeab567bef77706c7ff2e0cbfe0d77eb1L3489) in #13880.

…sagesByTimestamp (apache#13880)

### Motivation This issue was triggered by apache#13880, it doesn't handle the NPE in [here](https://github.com/apache/pulsar/pull/13880/files#diff-66aeb65a64cbe7c541f013ae807c5bcbeab567bef77706c7ff2e0cbfe0d77eb1R3502), we can check the [diff code](https://github.com/apache/pulsar/pull/13880/files#diff-66aeb65a64cbe7c541f013ae807c5bcbeab567bef77706c7ff2e0cbfe0d77eb1L3489) in apache#13880.

This issue was triggered by #13880, it doesn't handle the NPE in [here](https://github.com/apache/pulsar/pull/13880/files#diff-66aeb65a64cbe7c541f013ae807c5bcbeab567bef77706c7ff2e0cbfe0d77eb1R3502), we can check the [diff code](https://github.com/apache/pulsar/pull/13880/files#diff-66aeb65a64cbe7c541f013ae807c5bcbeab567bef77706c7ff2e0cbfe0d77eb1L3489) in #13880. (cherry picked from commit a3c8525)

fix call sync in async internalExpireMessagesByTimestamp

793eeb1

mattisonchao mentioned this pull request Jan 21, 2022

[Broker] PersistentTopicsBase has many async method call sync method. #13854

Closed

26 tasks

github-actions bot assigned liudezhi2098 Jan 21, 2022

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 21, 2022

codelipenghui added this to the 2.10.0 milestone Jan 21, 2022

codelipenghui added release/2.8.3 release/2.9.3 labels Jan 21, 2022

codelipenghui reviewed Jan 21, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated Show resolved Hide resolved

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated Show resolved Hide resolved

alert method name.

c88b81a

Jason918 reviewed Jan 21, 2022

View reviewed changes

liudezhi2098 added 2 commits January 22, 2022 10:14

internalExpireMessagesByTimestampForSinglePartition implemented by th…

4bc56fe

…is async method.

validateTopicOperationAsync put It in the front

3665568

liudezhi2098 added 2 commits January 22, 2022 20:07

Missed exception handling

b436168

fix some problems.

af45077

liudezhi2098 mentioned this pull request Jan 23, 2022

[Issue 13854][Broker]Fix call sync method in async rest api for internalExpireMessagesForA… #13905

Closed

2 tasks

AnonHxy reviewed Jan 23, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Show resolved Hide resolved

Jason918 mentioned this pull request Jan 23, 2022

[broker] Optimize topic policy with HierarchyTopicPolicies #13756

Closed

23 tasks

fix internalExpireMessagesForAllSubscriptionsForNonPartitionedTopic t…

83bc406

…o async

liudezhi2098 requested review from Jason918 and codelipenghui January 26, 2022 03:59

Jason918 reviewed Jan 26, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Show resolved Hide resolved

take advice

a810206

liudezhi2098 requested a review from Jason918 January 26, 2022 08:10

Technoboy- reviewed Jan 26, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated Show resolved Hide resolved

Technoboy- reviewed Jan 26, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated Show resolved Hide resolved

Technoboy- reviewed Jan 26, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated Show resolved Hide resolved

liudezhi2098 added 2 commits January 26, 2022 18:45

take advice

75aae55

take advice

6ee08c3

Jason918 reviewed Jan 27, 2022

View reviewed changes

liudezhi2098 added 4 commits January 27, 2022 12:08

take advice

7b7c536

handle conflict

7f1f76c

Merge branch 'master' of https://github.com/apache/pulsar into apache…

30c1452

…-master handle conflict

Merge branch 'apache-master' into fix_call_sync_in_async_internalExpi…

c483851

…reMessagesByTimestamp handle conflict

liudezhi2098 requested review from Jason918 and Technoboy- January 27, 2022 05:20

Jason918 approved these changes Jan 27, 2022

View reviewed changes

codelipenghui approved these changes Jan 27, 2022

View reviewed changes

codelipenghui merged commit d1d169e into apache:master Jan 27, 2022

codelipenghui removed release/2.8.3 release/2.9.3 labels Jan 29, 2022

nodece mentioned this pull request Feb 11, 2022

[Broker] Fix NPE of internalExpireMessagesByTimestamp #14243

Merged

2 tasks

Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022

[Broker] Fix call sync method in async rest api for internalExpireMes…

6cf3359

…sagesByTimestamp (apache#13880)

Technoboy- mentioned this pull request Apr 22, 2022

[improve][admin] Put validateTopicOwnershipAsync before validateTopicOperationAsync #15265

Merged

1 task

[Broker] Fix call sync method in async rest api for internalExpireMessagesByTimestamp #13880

[Broker] Fix call sync method in async rest api for internalExpireMessagesByTimestamp #13880

Uh oh!

Conversation

liudezhi2098 commented Jan 21, 2022

Motivation

Modifications

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

Documentation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liudezhi2098 commented Jan 22, 2022

Uh oh!

liudezhi2098 commented Jan 23, 2022

Uh oh!

Uh oh!

liudezhi2098 commented Jan 24, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattisonchao commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liudezhi2098 commented Jan 26, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liudezhi2098 commented Jan 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mattisonchao commented Jan 26, 2022 •

edited

Loading