Move exception handling from Google Bigtable operators to hooks

[KamranImaaz]

Fixes #60687

[KamranImaaz]

@shahar1 due to a small mistake i accidentally pushed the all commits and got auto assigned reviewers. Sorry for that.

[shahar1]

Not bad!
Please fix according to the comments.

[KamranImaaz]

And Also Shahar this we are using everywhere in the operator instead of that we can use instance.exists() in one line in hooks right??

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

[shahar1]

instance = hook.get_instance(project_id=self.project_id, instance_id=self.instance_id)

You may encapsulate it in the hook where applicable

[KamranImaaz]

Okay Sure

[KamranImaaz]

I have updated the code according to your comments. Please have a view.

[KamranImaaz]

I created an issue for that yesterday inorder to get your approval before opening a PR. Please have a look #61269

I wasn't talking about the documentation in bigtable.rst (which indeed should be updated), but the docstring which is part of the method. Take a look at: https://github.com/KamranImaaz/apache.airflow/blob/79fb50b12d738f75f25f29717b92a16b75cb22dc/providers/google/src/airflow/providers/google/cloud/hooks/bigtable.py#L245

After your changes this won't be true anymore (same goes for strings in the other methods). I hope that it's clearer now.

Done Shahar Updated the docstrings. Thanks

[shahar1]

Excellent job Kamran!

CC: @VladaZakharova @MaksYermak

[KamranImaaz]

Excellent job Kamran!

CC: @VladaZakharova @MaksYermak

Thanks Shahar

[KamranImaaz]

There are many open GCP issues(some are very older that i don't know whether they are relevant or not). If you don't mind can you assign me any of those issues. I would be happy to work on those.

[shahar1]

There are many open GCP issues(some are very older that i don't know whether they are relevant or not). If you don't mind can you assign me any of those issues. I would be happy to work on those.

Sounds good, please tag me in a couple of issues and I'll assign you if they are still relevant.

[shahar1]

Static checks seems to fail, could you please run the pre-commits?

[KamranImaaz]

There are many open GCP issues(some are very older that i don't know whether they are relevant or not). If you don't mind can you assign me any of those issues. I would be happy to work on those.

Sounds good, please tag me in a couple of issues and I'll assign you if they are still relevant.

Sure I will tag you

[KamranImaaz]

@shahar1 All Checks have passed!

[shahar1]

@shahar1 All Checks have passed!

Great! I'll give Google's team a couple of more days to comment, and then I'll merge.

[KamranImaaz]

Shahar I have one suggestion. In Bigtable Hooks currently we are having this get_cluster_states_for_table (it fetches the clusters associated with a particular table) but which is currently not used by any Operator.

For BigtableCreateTableOperator, since clusters are created by default while creating a table, would it make sense to reuse this hook method and log the number (and possibly state) of clusters associated with the table after creation?

Whats your view in this??

[shahar1]

get_cluster_states_for_table

Shahar I have one suggestion. In Bigtable Hooks currently we are having this get_cluster_states_for_table (it fetches the clusters associated with a particular table) but which is currently not used by any Operator.

For BigtableCreateTableOperator, since clusters are created by default while creating a table, would it make sense to reuse this hook method and log the number (and possibly state) of clusters associated with the table after creation?

Whats your view in this??

It's being used in the BigtableTableReplicationCompletedSensor sensor.
I don't think it makes sense to be used in BigtableCreateTableOperator, as clusters are not created by default (you run hook.get_instance() to get them, they are not auto-provisioned).

[shahar1]

Following a recent incident - I want also to ensure that related system tests pass before merging this one.
@KamranImaaz - if you're able to run them, please go ahead and attach screenshots. Otherwise, we'll have to wait for either Google team or anyone else to run them.

CC: @VladaZakharova @KamranImaaz

[KamranImaaz]

Following a recent incident - I want also to ensure that related system tests pass before merging this one. @KamranImaaz - if you're able to run them, please go ahead and attach screenshots. Otherwise, we'll have to wait for either Google team or anyone else to run them.

CC: @VladaZakharova @KamranImaaz

Hi @shahar1 while I currently do not have the access to run the system tests. I will try to get the access and run them. In the meanwhile google team can run them

[MaksYermak]

LGTM

[shahar1]

LGTM

Just to make it clear - are you ok with merging without running the system tests?
Personally I don't find anything that could be broken in the system tests due to the suggested changes, but it was @VladaZakharova 's request - so I want to ensure that we're all aligned.
If you are able to run it on your side before merging it would be the best, so we could avoid unnecessary reverts later on.
I'm also still waiting to chat with your team regarding the integration of Google provider system tests into the CI.
Thanks!

[shahar1]

Ran it myself eventually, we're good to go:

@KamranImaaz - Great job!

[KamranImaaz]

Thanks