-
Notifications
You must be signed in to change notification settings - Fork 330
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
[#4226] Improvement(python-client): Add Metalake Error Handler and related exceptions, test cases in client-python #4271
Conversation
Hi @jingjia88, thanks for your contribution. There seems to be some conflicts between this PR and main branch. |
@noidname01 Thanks for reminding. Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jingjia88 It seems that you don't use this error handler in methods in gravitino_metalake.py
and related files.
And please run the tests locally and make sure the tests you write would pass in your local environment.
@noidname01 Hi, I have made the changes and run the tests. Could you please review the changes again? Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use CatalogErrorHandler
in gravitino_metalake.py
, not MetalakeErrorHandler
.
@noidname01 Oops, I agree we should use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jingjia88 It seems that your code haven't pass black
formatter before commit, please format your code.
@@ -54,6 +54,9 @@ class InternalError(GravitinoRuntimeException): | |||
class NotFoundException(GravitinoRuntimeException): | |||
"""Base class for all exceptions thrown when a resource is not found.""" | |||
|
|||
def __init__(self, message, *args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the __init__
function here is not necessary, because the parent class (GravitinoRuntimeException
here) has implemented, except we want some custom features for this class only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed!
@noidname01 Done! Please help to take a look again, Thank you~ |
LGTM. |
What changes were proposed in this pull request?
client-python
based onclient-java
client-java
Why are the changes needed?
Fix: #4226
Does this PR introduce any user-facing change?
No.
How was this patch tested?
UT and IT added.