Skip to content

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

Merged
codelipenghui merged 14 commits into
apache:masterfrom
liudezhi2098:fix_call_sync_in_async_internalExpireMessagesByTimestamp
Jan 27, 2022
Merged

[Broker] Fix call sync method in async rest api for internalExpireMessagesByTimestamp#13880
codelipenghui merged 14 commits into
apache:masterfrom
liudezhi2098:fix_call_sync_in_async_internalExpireMessagesByTimestamp

Conversation

@liudezhi2098

Copy link
Copy Markdown
Contributor

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

@liudezhi2098

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@liudezhi2098

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@liudezhi2098

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

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

mattisonchao commented Jan 26, 2022

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@liudezhi2098

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@codelipenghui
codelipenghui merged commit d1d169e into apache:master Jan 27, 2022
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants