[improve][client] Throw more meaningful derived exception instead of PulsarClientException #15594

Shawyeok · 2022-05-13T15:37:29Z

Motivation

Currently we are abuse PulsarClientException, I think we could use a more meaningful derived exception instead. For example, call acknowledge on an already closed consumer throws PulsarClientException with message Consumer already closed, it's hard for caller to determine what kind of exception they catch.

Modifications

It's safe to use a derived exception instead of PulsarClientException.

Verifying this change

Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

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

Check the box below or label this PR directly.

Need to update docs?

doc-not-needed

Matching PR in forked repository

PR in forked repository: Shawyeok#3

merlimat

Looks good. We should probably also have assertions in the tests to ensure these proper exception are thrown, so that we don't have regression cases.

Shawyeok · 2022-05-13T16:47:39Z

Looks good. We should probably also have assertions in the tests to ensure these proper exception are thrown, so that we don't have regression cases.

Ok, I'll add unit test later.

Shawyeok · 2022-05-14T11:24:06Z

Looks good. We should probably also have assertions in the tests to ensure these proper exception are thrown, so that we don't have regression cases.

@merlimat Unit test added, PTAL

Shawyeok · 2022-05-14T13:55:19Z

/pulsarbot rerun-failure-checks

Shawyeok · 2022-05-23T16:51:48Z

/pulsarbot rerun-failure-checks

Shawyeok · 2022-05-24T02:13:22Z

/pulsarbot rerun-failure-checks

Shawyeok · 2022-05-24T17:32:37Z

@lhotari @codelipenghui PTAL

github-actions · 2022-06-24T02:16:19Z

The pr had no activity for 30 days, mark with Stale label.

github-actions · 2022-10-12T17:36:00Z

@Shawyeok Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

Shawyeok · 2022-11-01T10:18:25Z

/pulsarbot rerun-failure-checks

pulsar-client/src/test/java/org/apache/pulsar/client/impl/MultiTopicsConsumerImplTest.java

tisonkun · 2022-11-14T06:13:15Z

/pulsarbot run-failure-checks

eolivelli · 2022-11-14T07:08:16Z

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

-                                        log.warn("Failed to authorize {} on topic {}", clientAppId, topicName);
-                                        authorizationFuture.completeExceptionally(new PulsarClientException(
-                                                String.format("Authorization failed %s on topic %s with error %s",
-                                                clientAppId, topicName, throwable2.getMessage())));


This is in the broker.
I thinks that we should split the path per component

This is in the broker. I thinks that we should split the path per component

Sorry, could you please tell more specific? I'm guessing you are telling we shouldn't use exception class (PulsarClientException in here) that defined in the another component? Am I right?

@eolivelli Could you explain in details? I'm also wondering what did you mean by split the path per component.

codecov-commenter · 2022-11-14T16:20:23Z

Codecov Report

❗ No coverage uploaded for pull request base (master@92b4708). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #15594   +/-   ##
=========================================
  Coverage          ?   50.63%           
  Complexity        ?     6992           
=========================================
  Files             ?      383           
  Lines             ?    41845           
  Branches          ?     4272           
=========================================
  Hits              ?    21190           
  Misses            ?    18395           
  Partials          ?     2260

Flag	Coverage Δ
unittests	`50.63% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Shawyeok · 2022-11-17T14:53:17Z

/pulsarbot rerun-failure-checks

github-actions · 2022-12-18T01:59:13Z

The pr had no activity for 30 days, mark with Stale label.

poorbarcode · 2023-04-10T08:25:02Z

Since we will start the RC version of 3.0.0 on 2023-04-11, I will change the label/milestone of PR who have not been merged.

The PR of type feature is deferred to 3.1.0
The PR of type fix is deferred to 3.0.1

So drag this PR to 3.0.1

merlimat reviewed May 13, 2022

View reviewed changes

github-actions bot assigned Shawyeok May 13, 2022

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

Technoboy- added this to the 2.11.0 milestone May 14, 2022

Technoboy- added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/client labels May 14, 2022

Shawyeok requested a review from merlimat May 14, 2022 11:23

Shawyeok force-pushed the fix-throw-exception-type branch from 853a490 to 2020ab7 Compare May 15, 2022 04:27

Shawyeok force-pushed the fix-throw-exception-type branch 2 times, most recently from 2f07608 to 97527e2 Compare May 24, 2022 14:18

github-actions bot added the Stale label Jun 24, 2022

codelipenghui modified the milestones: 2.11.0, 2.12.0 Jul 26, 2022

Throw more meaningful derived exception instead of PulsarClientException

68be8ed

Shawyeok force-pushed the fix-throw-exception-type branch from 97527e2 to 2126c83 Compare October 12, 2022 17:01

Shawyeok changed the title ~~[pulsar-client] Throw more meaningful derived exception instead of PulsarClientException~~ [improve][pulsar-client] Throw more meaningful derived exception instead of PulsarClientException Oct 12, 2022

Shawyeok changed the title ~~[improve][pulsar-client] Throw more meaningful derived exception instead of PulsarClientException~~ [improve][client] Throw more meaningful derived exception instead of PulsarClientException Oct 12, 2022

Shawyeok force-pushed the fix-throw-exception-type branch from 2126c83 to a6d6648 Compare October 12, 2022 17:30

github-actions bot added doc-label-missing and removed doc-not-needed Your PR changes do not impact docs labels Oct 12, 2022

Add unit test

3660da4

Shawyeok force-pushed the fix-throw-exception-type branch from a6d6648 to 3660da4 Compare October 12, 2022 17:53

codelipenghui approved these changes Nov 1, 2022

View reviewed changes

pulsar-client/src/test/java/org/apache/pulsar/client/impl/MultiTopicsConsumerImplTest.java Outdated Show resolved Hide resolved

codelipenghui added the ready-to-test label Nov 1, 2022

tisonkun approved these changes Nov 14, 2022

View reviewed changes

eolivelli requested changes Nov 14, 2022

View reviewed changes

Shawyeok added 2 commits November 14, 2022 15:42

Apply code review suggestion

e11f9d5

Address checkstyle issue

1dec1df

github-actions bot added the Stale label Dec 18, 2022

tisonkun requested a review from eolivelli December 18, 2022 02:15

poorbarcode added the release/3.0.1 label Apr 10, 2023

poorbarcode modified the milestones: 3.0.0, 3.1.0 Apr 10, 2023

RobertIndie added release/3.0.1 release/3.0.2 and removed release/3.0.1 labels Jun 8, 2023

Technoboy- modified the milestones: 3.1.0, 3.2.0 Jul 31, 2023

poorbarcode added release/3.0.3 and removed release/3.0.2 labels Oct 24, 2023

Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023

coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024

Shawyeok closed this Aug 8, 2024

[improve][client] Throw more meaningful derived exception instead of PulsarClientException #15594

[improve][client] Throw more meaningful derived exception instead of PulsarClientException #15594

Uh oh!

Conversation

Shawyeok commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

Uh oh!

merlimat left a comment

Choose a reason for hiding this comment

Uh oh!

Shawyeok commented May 13, 2022

Uh oh!

Shawyeok commented May 14, 2022

Uh oh!

Shawyeok commented May 14, 2022

Uh oh!

Shawyeok commented May 23, 2022

Uh oh!

Shawyeok commented May 24, 2022

Uh oh!

Shawyeok commented May 24, 2022

Uh oh!

github-actions bot commented Jun 24, 2022

Uh oh!

github-actions bot commented Oct 12, 2022

Uh oh!

Shawyeok commented Nov 1, 2022

Uh oh!

Uh oh!

tisonkun commented Nov 14, 2022

Uh oh!

eolivelli Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

Shawyeok Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

BewareMyPower Dec 18, 2022

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Shawyeok commented Nov 17, 2022

Uh oh!

github-actions bot commented Dec 18, 2022

Uh oh!

poorbarcode commented Apr 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Shawyeok commented May 13, 2022 •

edited

Loading

codecov-commenter commented Nov 14, 2022 •

edited

Loading