shahar1 commented on code in PR #61008:
URL: https://github.com/apache/airflow/pull/61008#discussion_r2724255311
##########
providers/google/src/airflow/providers/google/cloud/hooks/bigtable.py:
##########
@@ -252,7 +253,10 @@ 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)
+ try:
+ table = instance.table(table_id=table_id)
+ except google.api_core.exceptions.NotFound:
+ self.log.info("The table '%s' no longer exists. Consider it as
deleted", table_id)
table.delete()
Review Comment:
This will fail if `NotFound` was previously caught. Maybe it's the
`table.delete()` that you want to handle exceptions for.
Try to get familiar with what each call returns (read the official API) and
decide according to that.
##########
providers/google/src/airflow/providers/google/cloud/operators/bigtable.py:
##########
@@ -600,16 +581,8 @@ def execute(self, context: Context) -> None:
impersonation_chain=self.impersonation_chain,
)
instance = hook.get_instance(project_id=self.project_id,
instance_id=self.instance_id)
- if not instance:
- raise AirflowException(f"Dependency: instance '{self.instance_id}'
does not exist.")
-
- try:
+ if instance:
hook.update_cluster(instance=instance, cluster_id=self.cluster_id,
nodes=self.nodes)
BigtableClusterLink.persist(context=context)
- except google.api_core.exceptions.NotFound:
- raise AirflowException(
- f"Dependency: cluster '{self.cluster_id}' does not exist for
instance '{self.instance_id}'."
- )
- except google.api_core.exceptions.GoogleAPICallError as e:
- self.log.error("An error occurred. Exiting.")
- raise e
+ else:
+ raise AirflowException(f"Dependency: instance '{self.instance_id}'
does not exist.")
Review Comment:
Actually it's better to leave this as an early exit-pattern as it was before:
```python
if not instance:
raise AirflowException(f"Dependency: instance
'{self.instance_id}' does not exist.")
```
While the `else` statement in your change achieves the same effect, the
former is better for readability.
--
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]
