Remove redundant try/except blocks in BigtableDeleteInstanceOperator

[KamranImaaz]

Fix #60687
Have Checked the Code and Modified accordingly also checked when the instance is not available to delete it should not log an error(Idempotent).

Tested Working Fine from My Side

[KamranImaaz]

Hi @shahar1 Can you review it once. If it's good from your side.

[shahar1]

Overall LGTM, small comment.

Also, could you please apply the same fix to the try..except block of BigtableDeleteTableOperator? (same principle)

@VladaZakharova I'll be happy for your review here as well - allegedly both Delete operators should never reach the NotFound case as it is already handled in the hook (it's a 7 years old code, so maybe things have been a bit different in the past). Please ignore, apologies for the hussle.

[KamranImaaz]

Overall LGTM, small comment.

Also, could you please apply the same fix to the try..except block of BigtableDeleteTableOperator? (same principle)

@VladaZakharova I'll be happy for your review here as well - allegedly both Delete operators should never reach the NotFound case as it is already handled in the hook (it's a 7 years old code, so maybe things have been a bit different in the past).

Yes Once it is approved I will raise another PR and modify the Bigtable.py file.

[KamranImaaz]

@shahar1 not only delete operator but execute method in every class is having try and except block. will remove all in One PR? or Individual PR is required for every class in Bigtable.py?

[shahar1]

@shahar1 not only delete operator but execute method in every class is having try and except block. will remove all in One PR? or Individual PR is required for every class in Bigtable.py?

For this PR - you could also insert the other class (there is only one).
For other files (bigquery, etc.) - please a separate PR for each.

[KamranImaaz]

@shahar1 not only delete operator but execute method in every class is having try and except block. will remove all in One PR? or Individual PR is required for every class in Bigtable.py?

For this PR - you could also insert the other class (there is only one). For other files (bigquery, etc.) - please a separate PR for each.

Okay Sure

[KamranImaaz]

@shahar1 Apologies for Late Updates. Also in Bigtable.py there are still many classes such as BigtableCreateInstanceOperator, BigtableUpdateInstanceOperator, BigtableCreateTableOperator, BigtableUpdateClusterOperator which are having redundant try..except blocks. If you are Ok with it I will modify in this PR itself since all these classes are in bigtable.py itself. Waiting for your confirmation!

[shahar1]

Hey @KamranImaaz , I went over the PR more thoroughly and I think that I've accidentally misled you -
I had initially thought that we use try..except in both the operators and hooks (which is indeed not necessary), but as it seems - the hook actually calls the API straight forward, and then the operators use try..except to reach idempotency in action, which is already the desired behavior.
When you delete a table and the instance does not exist, it rightously raises a RuntimeError error - because the instance is assumed to exist so you would be able to delete the table (whether it exists or not). If you run the Delete table operator, and the instance does not exist - it means that this assumption is wrong (regardless of the idempotency of the table), and the user needs to be alerted about it as a failure of the operator.
I sincerely apologize for missing it - but unfortunately it seems that we won't get too much from this one.
I'm aware that it's important for you to get your 3rd PR merged soon - so please try to look for existing issues labeled as good first issue, and let me know.

[shahar1]

btw, there might be a case for the other operator (DeleteInstance) to remove handling of NotFound resulting from the delete_instance (because the latter handles it internally in the hook) - but currently it seems that someone else opened a PR for that: #60727.

[KamranImaaz]

Hey @KamranImaaz , I went over the PR more thoroughly and I think that I've accidentally misled you - I had initially thought that we use try..except in both the operators and hooks (which is indeed not necessary), but as it seems - the hook actually calls the API straight forward, and then the operators use try..except to reach idempotency in action, which is already the desired behavior. When you delete a table and the instance does not exist, it rightously raises a RuntimeError error - because the instance is assumed to exist so you would be able to delete the table (whether it exists or not). If you run the Delete table operator, and the instance does not exist - it means that this assumption is wrong (regardless of the idempotency of the table), and the user needs to be alerted about it as a failure of the operator. I sincerely apologize for missing it - but unfortunately it seems that we won't get too much from this one. I'm aware that it's important for you to get your 3rd PR merged soon - so please try to look for existing issues labeled as good first issue, and let me know.

Yes @shahar1 That doubt I was having initially. That's why I did not pushed it in initial commit as I specifically raised it for Instance operator

[KamranImaaz]

I have reverted the changes for delete table operator. Kindly verify it

[KamranImaaz]

Hi @shahar1 All CI checks have passed. Can you please review it once

[shahar1]

LGTM

CC: @VladaZakharova @MaksYermak

1. For the NotFound case the hook takes care of idempotency already - I don't see a reason to duplicate this logic in the operator as it never actually reaches there (YAGNI).
2. The second exception seems redundant.

[shahar1]

Hi @shahar1 All CI checks have passed. Can you please review it once

I have reviewed it. Please note that there's no need to tag me in each comment - once I'm tagged as reviewer, I get notified for every change and comment in the PR.
I'll let Google's team a couple of days to review it before merging - in the meanwhile, you could work on the other operators or start working on new PRs labeled as good first issues.

[KamranImaaz]

Hi @shahar1 All CI checks have passed. Can you please review it once

I have reviewed it. Please note that there's no need to tag me in each comment - once I'm tagged as reviewer, I get notified for every change and comment in the PR. I'll let Google's team a couple of days to review it before merging - in the meanwhile, you could work on the other operators or start working on new PRs labeled as good first issues.

I Generally have the habit of tagging. But I'll follow your advice. I Apologize for the inconvinience caused.

[MaksYermak]

Hello @KamranImaaz thank you for your PR!
You mentioned in the issue 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 and get_instance. 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 could 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?

[KamranImaaz]

Hello @KamranImaaz thank you for your PR! You mentioned in the issue 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 and get_instance. 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 could 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

[shahar1]

Hello @KamranImaaz thank you for your PR! You mentioned in the issue 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 and get_instance. 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 could 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 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 and get_instance. 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 could 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:

        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).

[MaksYermak]

@KamranImaaz @shahar1 thank you for the explanation!
My fault now I see that instance() is just a construct and, also, I see that we handle NotFound exception on the Client side inside exists method here .
This PR looks good to me, thank you one more time for your clarification

[shahar1]

@KamranImaaz great job! Looking forward for more PRs.

[KamranImaaz]

Thanks @shahar1 I will raise different PR for other operators and link it to the same issue. And it was nice working with you. I learned a lot from you. Once again Apologize for any inconvenience caused to you.
Thank you have a great day!

[KamranImaaz]

Thanks @MaksYermak for your review.