Skip to content

Refactor Bigtable Delete Instance Operator to remove redundant try/excep… #60727

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.

Sign up for GitHub

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

Closed
Jayyadav1903 wants to merge 3 commits into from
Closed

Refactor Bigtable Delete Instance Operator to remove redundant try/excep… #60727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0fa3d75
Refactor BigtableDeleteInstanceOperator to remove redundant try/excep…
Jayyadav1903 Jan 17, 2026
33b2e0e
Update BigtableHook docstring and add PR description (#60687)
Jayyadav1903 Jan 17, 2026
1664689
Merge branch 'main' into fix-bigtable-redundant-except-60687
Jayyadav1903 Jan 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions PR_DESCRIPTION.md
View file
Open in desktop
Copy link
Contributor

@sjyangkevin sjyangkevin Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@jroachgolf84 jroachgolf84 Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is not needed and should be removed.

Copy link
Contributor

@shahar1 shahar1 Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to the above

Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
Closes: #60687

This PR removes redundant `try/except` blocks in `BigtableDeleteInstanceOperator` in `airflow/providers/google/cloud/operators/bigtable.py`.

The underlying `BigtableHook.delete_instance` method already handles the case where the instance does not exist by logging a warning. Therefore, catching `NotFound` in the operator was unnecessary and duplicated logic.

I have also:
- Updated the unit tests in `providers/google/tests/unit/google/cloud/operators/test_bigtable.py` to remove tests that specifically enforced this redundant exception handling.
- Corrected the docstring in `airflow/providers/google/cloud/hooks/bigtable.py` which incorrectly stated that `delete_instance` raises `NotFound`.

Generated-by: Antigravity (Google DeepMind)
3 changes: 1 addition & 2 deletions providers/google/src/airflow/providers/google/cloud/hooks/bigtable.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,7 @@ def delete_instance(self, instance_id: str, project_id: str) -> None:
"""
Delete the specified Cloud Bigtable instance.

Raises google.api_core.exceptions.NotFound if the Cloud Bigtable instance does
not exist.
If the Cloud Bigtable instance does not exist, it logs a warning.

:param project_id: Optional, Google Cloud project ID where the
BigTable exists. If set to None or missing,
Expand Down
12 changes: 1 addition & 11 deletions providers/google/src/airflow/providers/google/cloud/operators/bigtable.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,17 +331,7 @@ def execute(self, context: Context) -> None:
gcp_conn_id=self.gcp_conn_id,
impersonation_chain=self.impersonation_chain,
)
try:
hook.delete_instance(project_id=self.project_id, instance_id=self.instance_id)
except google.api_core.exceptions.NotFound:
self.log.info(
"The instance '%s' does not exist in project '%s'. Consider it as deleted",
self.instance_id,
self.project_id,
)
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)
Copy link
Contributor

@sjyangkevin sjyangkevin Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@shahar1 shahar1 Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



class BigtableCreateTableOperator(GoogleCloudBaseOperator, BigtableValidationMixin):
Expand Down
41 changes: 0 additions & 41 deletions providers/google/tests/unit/google/cloud/operators/test_bigtable.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,47 +583,6 @@ def test_empty_attribute(self, mock_hook, missing_attribute, project_id, instanc
assert str(err) == f"Empty parameter: {missing_attribute}"
mock_hook.assert_not_called()

@mock.patch("airflow.providers.google.cloud.operators.bigtable.BigtableHook")
def test_deleting_instance_that_doesnt_exists(self, mock_hook):
op = BigtableDeleteInstanceOperator(
project_id=PROJECT_ID,
instance_id=INSTANCE_ID,
task_id="id",
gcp_conn_id=GCP_CONN_ID,
impersonation_chain=IMPERSONATION_CHAIN,
)
mock_hook.return_value.delete_instance.side_effect = mock.Mock(
side_effect=google.api_core.exceptions.NotFound("Instance not found.")
)
op.execute(None)
mock_hook.assert_called_once_with(
gcp_conn_id=GCP_CONN_ID,
impersonation_chain=IMPERSONATION_CHAIN,
)
mock_hook.return_value.delete_instance.assert_called_once_with(
project_id=PROJECT_ID, instance_id=INSTANCE_ID
)

@mock.patch("airflow.providers.google.cloud.operators.bigtable.BigtableHook")
def test_deleting_instance_that_doesnt_exists_empty_project_id(self, mock_hook):
op = BigtableDeleteInstanceOperator(
instance_id=INSTANCE_ID,
task_id="id",
gcp_conn_id=GCP_CONN_ID,
impersonation_chain=IMPERSONATION_CHAIN,
)
mock_hook.return_value.delete_instance.side_effect = mock.Mock(
side_effect=google.api_core.exceptions.NotFound("Instance not found.")
)
op.execute(None)
mock_hook.assert_called_once_with(
gcp_conn_id=GCP_CONN_ID,
impersonation_chain=IMPERSONATION_CHAIN,
)
mock_hook.return_value.delete_instance.assert_called_once_with(
project_id=None, instance_id=INSTANCE_ID
)

@mock.patch("airflow.providers.google.cloud.operators.bigtable.BigtableHook")
def test_different_error_reraised(self, mock_hook):
op = BigtableDeleteInstanceOperator(
Expand Down
Loading