-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Apply flake8-logging-format changes to providers #24933
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,8 +107,8 @@ def get_iam_execution_role(self) -> Dict: | |
| glue_execution_role = iam_client.get_role(RoleName=self.role_name) | ||
| self.log.info("Iam Role Name: %s", self.role_name) | ||
| return glue_execution_role | ||
| except Exception as general_error: | ||
| self.log.error("Failed to create aws glue job, error: %s", general_error) | ||
| except Exception: | ||
| self.log.error("Failed to create aws glue job.") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to use
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep. Sounds strange. We shoud catch specific exception that we know about and let all the rest buble up directly, there is no point in extra logging here unless we want to provide any "specific" information resulting in helping the user to react to some known exceptions. There is no point in logging meaningless log here - the user knows, Glue job failed to be created already and providing this extra line with no actual "Help" for the user and without instructions on what to do is borderline harrasing the user "Hello you already know we failed, so let us repeat it here").
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Generally if there was a Oof yeah @potiuk there are a lot of generic exceptions in these files. I'll clean them up. |
||
| raise | ||
|
|
||
| def initialize_job( | ||
|
|
@@ -129,8 +129,8 @@ def initialize_job( | |
| job_name = self.get_or_create_glue_job() | ||
| return glue_client.start_job_run(JobName=job_name, Arguments=script_arguments, **run_kwargs) | ||
|
|
||
| except Exception as general_error: | ||
| self.log.error("Failed to run aws glue job, error: %s", general_error) | ||
| except Exception: | ||
| self.log.error("Failed to run aws glue job.") | ||
| raise | ||
|
|
||
| def get_job_state(self, job_name: str, run_id: str) -> str: | ||
|
|
@@ -280,8 +280,8 @@ def get_or_create_glue_job(self) -> str: | |
| **self.create_job_kwargs, | ||
| ) | ||
| return create_job_response['Name'] | ||
| except Exception as general_error: | ||
| self.log.error("Failed to create aws glue job, error: %s", general_error) | ||
| except Exception: | ||
| self.log.error("Failed to create aws glue job.") | ||
| raise | ||
|
|
||
|
|
||
|
|
||
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.
Since this is a provider, I wonder if we should just switch to re-raise the original exception. Coercing the error to AirflowException offers no benefits at all
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.
Agree :). We used to use that pattern in the past but unless you use dedicated AirflowSkip or AirflowFailException it's better to raise the original 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.
Ah yes, this totally makes sense. Agreed. I'll do a clean sweep of all the provider files I'm touching for this as well.