shahar1 commented on code in PR #61124:
URL: https://github.com/apache/airflow/pull/61124#discussion_r2733329905
##########
providers/google/src/airflow/providers/google/cloud/hooks/bigtable.py:
##########
@@ -252,11 +253,15 @@ def delete_table(self, instance_id: str, table_id: str,
project_id: str) -> None
instance = self.get_instance(instance_id=instance_id,
project_id=project_id)
if instance is None:
raise RuntimeError(f"Instance {instance_id} did not exist; unable
to delete table {table_id}")
+
table = instance.table(table_id=table_id)
- table.delete()
+ try:
+ table.delete()
+ except google.api_core.exceptions.NotFound:
+ self.log.info("The table '%s' no longer exists. Consider it as
deleted", table_id)
Review Comment:
Please add unit tests (you could use the tests you just deleted from the
operator)
##########
providers/google/src/airflow/providers/google/cloud/hooks/bigtable.py:
##########
@@ -252,11 +253,15 @@ def delete_table(self, instance_id: str, table_id: str,
project_id: str) -> None
instance = self.get_instance(instance_id=instance_id,
project_id=project_id)
if instance is None:
raise RuntimeError(f"Instance {instance_id} did not exist; unable
to delete table {table_id}")
+
table = instance.table(table_id=table_id)
- table.delete()
+ try:
+ table.delete()
+ except google.api_core.exceptions.NotFound:
+ self.log.info("The table '%s' no longer exists. Consider it as
deleted", table_id)
@staticmethod
- def update_cluster(instance: Instance, cluster_id: str, nodes: int) ->
None:
+ def update_cluster(instance: Instance, instance_id: str, cluster_id: str,
nodes: int) -> None:
Review Comment:
Adding a required parameter here is a breaking change.
You should be able to fetch the id from the `instance` parameter.
##########
providers/google/src/airflow/providers/google/cloud/hooks/bigtable.py:
##########
@@ -268,7 +273,12 @@ def update_cluster(instance: Instance, cluster_id: str,
nodes: int) -> None:
"""
cluster = Cluster(cluster_id, instance)
# "reload" is required to set location_id attribute on cluster.
- cluster.reload()
+ try:
+ cluster.reload()
+ except google.api_core.exceptions.NotFound:
+ raise AirflowException(
+ f"Dependency: cluster '{cluster_id}' does not exist for
instance '{instance_id}'."
+ )
Review Comment:
1. I'll waive the usage of `AirflowException` here because it was relocated
from the operator, but eventually we'll need to deprecate it.
2. If you relocate the logic - it should be tested (you could use the
deleted tests from the operator as a reference)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
