Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Issue 3127][python-client] Replace Exceptions with PulsarExceptions #7600

Merged
merged 10 commits into from
May 15, 2021

Conversation

lbenc135
Copy link
Contributor

@lbenc135 lbenc135 commented Jul 19, 2020

Fixes #7600

Motivation

As the issue says, the Python client throws Exceptions instead of a subclass of it (PulsarException), so the clients must catch the blanket Exception.

Modifications

Every C PulsarException is now thrown in Python with the same type.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Extended Python tests to check for PulsarExceptions

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: don't know

Documentation

  • Does this pull request introduce a new feature? no

@lbenc135
Copy link
Contributor Author

Hi @sijie @merlimat, could you please take a look?

@n0nvme
Copy link

n0nvme commented Mar 24, 2021

Is it possible to re-run all tests? Logs have expired and it's impossible to see what failed:(

@aahmed-se
Copy link
Contributor

@n0nvme rebase and resolve all conflicts first , then push those changes the tests will rerun.

@lbenc135 lbenc135 force-pushed the feature/pulsar_exception branch 3 times, most recently from 948c06a to 1566476 Compare March 25, 2021 07:13
@lbenc135
Copy link
Contributor Author

@n0nvme @aahmed-se I rebased and improved the PR a bit. Some tests are failing, although they seem flaky to me. Let me know if you have any comments.

@tuteng
Copy link
Member

tuteng commented Mar 25, 2021

@BewareMyPower PTAL

@tuteng tuteng requested a review from jiazhai March 25, 2021 23:26
@tuteng tuteng added component/python type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/client labels Mar 25, 2021
@tuteng tuteng added this to the 2.8.0 milestone Mar 25, 2021
self.assertTrue(False) # Should not reach this point
except:
pass # Exception is expected
with self.assertRaises(PulsarException):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In these tests, could we assert for pulsar.Timeout exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, refactored

@tuteng
Copy link
Member

tuteng commented Mar 27, 2021

/pulsarbot run-failure-checks

@BewareMyPower
Copy link
Contributor

@merlimat @tuteng @jiazhai Could we merge this PR currently?

@codelipenghui codelipenghui merged commit 3f36544 into apache:master May 15, 2021
@lbenc135 lbenc135 deleted the feature/pulsar_exception branch May 20, 2021 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/client type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants