Refactor Bigtable Delete Instance Operator to remove redundant try/excep… #60727
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
boring-cyborg
bot
added
area:providers
provider:google
|
Doesn’t PR #60712 already address this? |
There was a problem hiding this comment.
This looks like a file generated by the AI tool, we may not need this file here as it is the description for the PR.
There was a problem hiding this comment.
This file is not needed and should be removed.
| except google.api_core.exceptions.GoogleAPICallError as e: | ||
| self.log.error("An error occurred. Exiting.") | ||
| raise e | ||
| hook.delete_instance(project_id=self.project_id, instance_id=self.instance_id) |
There was a problem hiding this comment.
another case in the error handling except google.api_core.exceptions.GoogleAPICallError as e is also removed. In this case, an error need to be raised instead of showing a logging. It looks different from the PR description that only NotFound looks redundant.
There was a problem hiding this comment.
If such exception occurs, it will occur even without exception case, but without showing the error log "An error occured. Exiting.".
Whether the latter log error is really necessary - I doubt, but I think that it should be clarified within the PR description and title.
There was a problem hiding this comment.
Hello and welcome to Apache Airflow:
While I do appreciate the effort your put into creating this PR, I would like to draw your attention to two general notes:
- The issue that this PR resolves has been assigned to another contributor yesterday. As a general rule of thumb, it's best to wait a while (at least a week) before creating a PR for issue that has been assigned to someone else, as it’s not considered polite (and even a bit rude) to get in someone else’s way. You should ask in the GitHub issue if it's still in progress, and if the assignee responds that "no" or doesn't respond for a too long - then you may ask to be assigned instead.
- While use of AI is allowed, and you've even mentioned that you used it for this PR - it should not be trusted blindly and you also need to adhere to the project's standards. Adding the
PR_DESCRIPTION.mdto the root can signals to maintainers that something might be off.
I hope that you'll consider the above for your future contributions.
Please address the issues mentioned in this PR, including updating title and the description - I'll appreciate if you could try finalizing it without using AI (it's possible).
| except google.api_core.exceptions.GoogleAPICallError as e: | ||
| self.log.error("An error occurred. Exiting.") | ||
| raise e | ||
| hook.delete_instance(project_id=self.project_id, instance_id=self.instance_id) |
There was a problem hiding this comment.
If such exception occurs, it will occur even without exception case, but without showing the error log "An error occured. Exiting.".
Whether the latter log error is really necessary - I doubt, but I think that it should be clarified within the PR description and title.
|
This was my very first PR as I am a rookie in open source and would try to resolve all the issues at the earliest. |
It's alright, apologize accepted! |
|
I'll surely do that. |
5 participants
This PR removes redundant
try/exceptblocks inBigtableDeleteInstanceOperatorinairflow/providers/google/cloud/operators/bigtable.py.The underlying
BigtableHook.delete_instancemethod already handles the case where the instance does not exist by logging a warning. Therefore, catchingNotFoundin the operator was unnecessary and duplicated logic.I have also:
providers/google/tests/unit/google/cloud/operators/test_bigtable.pyto remove tests that specifically enforced this redundant exception handling.airflow/providers/google/cloud/hooks/bigtable.pywhich incorrectly stated thatdelete_instanceraisesNotFound.Generated-by: Antigravity (Google DeepMind)