Skip to content

Conversation

@lwyszomi
Copy link
Contributor

Refactor of the BigQueryToGCPOperator, now we using insert_job method not run_extract. Also adjustment for the links in the BigQueryInsertJobOperator

FYI @potiuk


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragement file, named {pr_number}.significant.rst, in newsfragments.

Copy link
Member

@turbaszek turbaszek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment regarding persisting links. Otherwise looks good to me.

conf = job["configuration"]["extract"]["sourceTable"]

try:
self.log.info(f"Executing: {configuration}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use %-formatting in logging so the formatting is deferred until the latest possible moment.

Comment on lines 2168 to 2171
if job_type in job.to_api_repr()["configuration"]:
for table_prop in tables_prop:
if table_prop in job.to_api_repr()["configuration"][job_type]:
table = job.to_api_repr()["configuration"][job_type][table_prop]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to write job.to_api_repr()["configuration"] 3 times? Could you please introduce a variable for this value?

@potiuk
Copy link
Member

potiuk commented Jun 14, 2022

Ah. How could I missed it !

@potiuk
Copy link
Member

potiuk commented Jun 14, 2022

Yeah. I think that last comment from @turbaszek and we can merge (and release 8.0.1)
BTW. @turbaszek - we want to relase 8.0.1 as this one fixes a serious problem in Google Provider 8.0.0 - so if you are not too strong with your comment, I'd merge and release it even now.

@potiuk
Copy link
Member

potiuk commented Jun 14, 2022

Soory @lwyszomi - it TOTALLY got under the radar - not sure how, I was waiting for that PR!

@potiuk
Copy link
Member

potiuk commented Jun 14, 2022

Ah yeah. there are also few comments from @josh-fell - so @lwyszomi -> if you can address them tomorrow morning, I am happy to wait.

@lwyszomi
Copy link
Contributor Author

@potiuk I pushed fixes for @josh-fell and @turbaszek comments.

@potiuk potiuk dismissed turbaszek’s stale review June 15, 2022 09:44

All addressed and we want to release quickly :)

@potiuk potiuk merged commit ce50d37 into apache:main Jun 15, 2022
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jul 11, 2022
Cherry-picked changes from the community:
* apache/airflow#24416
* apache/airflow#23078

Unit tests executed:
https://screenshot.googleplex.com/5AJvqmrXwoUA83L

Dags executed:
https://screenshot.googleplex.com/67ABr73jTfZETb7

Change-Id: I6f4795eed4a8ecc67b0f7997f228cd92d464d004
GitOrigin-RevId: 0fa3a13797a21cd5d6c3e08d315bb93f7af4e787
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants