-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
fix(ingestion): powerbi # continue ingestion if m-query parsing fail #7360
fix(ingestion): powerbi # continue ingestion if m-query parsing fail #7360
Conversation
except Exception as e: | ||
self.log_http_error( | ||
message=f"Unable to fetch users for {entity_name}({entity_id}).", e=e | ||
except: # It will catch all type of exception |
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.
What was the problem with the Exception as e
way of catching exception handling?
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.
+1 - I'm somewhat confused
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.
It is not catching ConnectionError and index-related exceptions, so Exception is not good if you want to catch all types of exceptions.
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.
Ah - thanks for clarification. This is useful to know!
except: # noqa: E722 | ||
# It will catch all type of exceptions, so that ingestion can continue without lineage information | ||
_, e, _ = sys.exc_info() | ||
logger.warning(str(e)) |
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.
Thank you- we need MORE of this.
logger.debug("flat_arg_list is zero") | ||
return None | ||
|
||
first_argument: Tree = flat_arg_list[0] # take first argument 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.
Thank you for adding this check. We need MORE of this!
) | ||
if data_resolver.is_permission_error(e): | ||
if data_resolver.is_permission_error(cast(Exception, e)): |
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.
Is this a safe cast if e is not actually an exception?
except Exception as e: | ||
self.log_http_error(message="Unable to fetch list of workspaces", e=e) | ||
except: | ||
self.log_http_error(message="Unable to fetch list of workspaces") |
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.
Won't we be losing this exception?
self.log_http_error( | ||
message=f"Unable to fetch reports for workspace {workspace.name}", e=e | ||
message=f"Unable to fetch reports for workspace {workspace.name}" |
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.
Won't we be losing this exception?
) | ||
if data_resolver.is_permission_error(e): | ||
if data_resolver.is_permission_error(cast(Exception, e)): |
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.
Is this cast safe? Given that e may not be an exception
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.
Want to get these changes for error handling out. Let's follow up on the dropping exceptions piece offline!
Cheers
John
…atahub-project#7360) Co-authored-by: MohdSiddique Bagwan <mohdsiddique.bagwan@gslab.com>
…7360) Co-authored-by: MohdSiddique Bagwan <mohdsiddique.bagwan@gslab.com>
No description provided.