-
Notifications
You must be signed in to change notification settings - Fork 3k
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(ingest/glue): add try catch #12449
Conversation
for tables in self.get_tables_from_database(database): | ||
all_tables.append(tables) | ||
except Exception as e: | ||
self.report.failure( |
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.
Do we know what kinds of exceptions are to happen here. Trying to determine if there are any that need to be reported as warnings instead.
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.
Access denied causing the whole ingestion to fail
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.
This self.report.failure
would cause ingestion to fail but will still do best effort metadata extraction.
If possible, it will be good to identify when its permission error and use more instructive and actionable message when reporting failure.
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 agree. This should have had been error reporting in the first place!
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
... and 37 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Checklist