Skip to content

[AIRFLOW-5817] Improve BigQuery operators idempotency#6470

Merged
kaxil merged 2 commits into
apache:masterfrom
PolideaInternal:gcp-bq-idempotence-dataset
Nov 7, 2019
Merged

[AIRFLOW-5817] Improve BigQuery operators idempotency#6470
kaxil merged 2 commits into
apache:masterfrom
PolideaInternal:gcp-bq-idempotence-dataset

Conversation

@turbaszek

@turbaszek turbaszek commented Oct 30, 2019

Copy link
Copy Markdown
Member

Make sure you have checked all steps below.

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:
    The PR adds idempotency to BigQueryCreateEmptyTableOperator and BigQueryCreateEmptyDatasetOperator. I added also comment to BigQueryOperator that it does not have to be idempotent.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

@turbaszek

Copy link
Copy Markdown
Member Author

@TobKed can you take a look?

@turbaszek
turbaszek force-pushed the gcp-bq-idempotence-dataset branch from cb77cef to 4ce1ec5 Compare October 30, 2019 13:34
@codecov-io

codecov-io commented Oct 30, 2019

Copy link
Copy Markdown

Codecov Report

Merging #6470 into master will increase coverage by 74.06%.
The diff coverage is 57.69%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #6470       +/-   ##
===========================================
+ Coverage    9.57%   83.64%   +74.06%     
===========================================
  Files         635      635               
  Lines       36679    36685        +6     
===========================================
+ Hits         3513    30685    +27172     
+ Misses      33166     6000    -27166
Impacted Files Coverage Δ
airflow/gcp/hooks/bigquery.py 70.19% <100%> (+70.19%) ⬆️
airflow/gcp/operators/bigquery.py 84.18% <50%> (+84.18%) ⬆️
airflow/kubernetes/volume_mount.py 44.44% <0%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0%> (-47.06%) ⬇️
airflow/kubernetes/pod_launcher.py 45.25% <0%> (-43.8%) ⬇️
airflow/kubernetes/kube_client.py 33.33% <0%> (-41.67%) ⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py 70.14% <0%> (-28.36%) ⬇️
airflow/plugins_manager.py 87.27% <0%> (+0.9%) ⬆️
airflow/config_templates/airflow_local_settings.py 80.48% <0%> (+2.43%) ⬆️
airflow/exceptions.py 100% <0%> (+3.7%) ⬆️
... and 572 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eeec376...e07c6f8. Read the comment docs.

Comment thread airflow/gcp/hooks/bigquery.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't get what changed! We just moved it from hook to operator !!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In operator in case of http error 409 we assume the resource already exists. Previously an error was thrown like "Database already exists", thus the operator was not idempotent.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Nearly, in case of http 409 we do not rise exception, because resource already exists.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

Comment thread airflow/gcp/hooks/bigquery.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This log message won't make sense if the table already exists or there is another HTTP error !

If someone just uses this hook and not the operator, the log message is incorrect. The try/catch should live in the hook I think

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch! I agree that the log should go to operator.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM once the logline is moved to Operator.

@mik-laj mik-laj added the provider:google Google (including GCP) related issues label Oct 31, 2019
@feluelle

feluelle commented Nov 1, 2019

Copy link
Copy Markdown
Member

@nuclearpinguin master is currently failing. A potential fix #6486 is already there. Therefore I cancelled your tests. Can you do a rebase after the PR is merged, please? :)

@turbaszek

Copy link
Copy Markdown
Member Author

@feluelle I will do 👌🏼

@turbaszek
turbaszek force-pushed the gcp-bq-idempotence-dataset branch from 529552e to a8f8088 Compare November 1, 2019 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants