shahar1 commented on PR #60712: URL: https://github.com/apache/airflow/pull/60712#issuecomment-3783088333
> Hello @KamranImaaz thank you for your PR! You mentioned in the [issue](https://github.com/apache/airflow/issues/60687) that we do not need this `try ... except` block in the Operator side because we handle it on the Hook side. I have checked the Hook code for `delete_instance` and `get_instance` methods and I do not see any `try...except` block in these methods which handles the `google.api_core.exceptions.NotFound` exception. Here is the code for [delete_instance](https://github.com/apache/airflow/blob/main/providers/google/src/airflow/providers/google/cloud/hooks/bigtable.py#L84C1-L105C1) and [get_instance](https://github.com/apache/airflow/blob/main/providers/google/src/airflow/providers/google/cloud/hooks/bigtable.py#L69C1-L85C1). As I see `get_instance` method has a `if-clause` statement which is checking `instance` for existing and returns `None` in case if `instance` does not exist and `delete_instance` relies on it and prints warning message in the logs. @KamranImaaz cou ld you please clarify if I missed something? > > In my opinion it is not the same as handling errors by the `try ... except` block. This line of the code: `instance = self._get_client(project_id=project_id).instance(instance_id)` still can produce `google.api_core.exceptions.NotFound` exception and I do not see any proves that removing `try ... execept` block does not break `BigtableDeleteInstanceOperator` operator. Because this `try ... execpt` block is needed for finishing the task successfully in case when `instance` is not found. @KamranImaaz could you please share with us the screenshots that the task which runs this operator has the same behavior as before and will not fail in case when the instance is not found? > > Hello @KamranImaaz thank you for your PR! You mentioned in the [issue](https://github.com/apache/airflow/issues/60687) that we do not need this `try ... except` block in the Operator side because we handle it on the Hook side. I have checked the Hook code for `delete_instance` and `get_instance` methods and I do not see any `try...except` block in these methods which handles the `google.api_core.exceptions.NotFound` exception. Here is the code for [delete_instance](https://github.com/apache/airflow/blob/main/providers/google/src/airflow/providers/google/cloud/hooks/bigtable.py#L84C1-L105C1) and [get_instance](https://github.com/apache/airflow/blob/main/providers/google/src/airflow/providers/google/cloud/hooks/bigtable.py#L69C1-L85C1). As I see `get_instance` method has a `if-clause` statement which is checking `instance` for existing and returns `None` in case if `instance` does not exist and `delete_instance` relies on it and prints warning message in the logs. @KamranImaaz c ould you please clarify if I missed something? > > In my opinion it is not the same as handling errors by the `try ... except` block. This line of the code: `instance = self._get_client(project_id=project_id).instance(instance_id)` still can produce `google.api_core.exceptions.NotFound` exception and I do not see any proves that removing `try ... execept` block does not break `BigtableDeleteInstanceOperator` operator. Because this `try ... execpt` block is needed for finishing the task successfully in case when `instance` is not found. @KamranImaaz could you please share with us the screenshots that the task which runs this operator has the same behavior as before and will not fail in case when the instance is not found? > > In the issue I mean that the exceptions raised at the Operator Level is already taken care in Hook Level(Checks the existence of instance or not i.e `if...else` blocks). > > Correct me if Iam wrong. Removing `try..except` block in the operator is safe because the hook already checks `instance.exists()` and returns `False` when the instance does not exist. `client.instance(instance_id)` does not make a API call. `instance.exists()` is the one who calls the API and it returns `False` +1 for that: ```python instance = self._get_client(project_id=project_id).instance(instance_id) if not instance.exists(): return None return instance ``` How can the first line produce a `NotFound` error if the next in line checks for existence? It just constructs the `Instance` object without calling the API (I checked also the nested calls, only assigning attributes). -- 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]
