Skip to content

[AIRFLOW-2590] Fix commit in DbApiHook.run() for no-autocommit DB #3485

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
KevinYang21 wants to merge 1 commit into from
Closed

[AIRFLOW-2590] Fix commit in DbApiHook.run() for no-autocommit DB #3485

KevinYang21 wants to merge 1 commit into from
+2 −1

Conversation

@KevinYang21
Copy link
Member

@KevinYang21 KevinYang21 commented Jun 10, 2018 •
edited
Loading

JIRA

  • My PR addresses the following Airflow JIRA issues and references them in the PR title.

Description

  • Here are some details about my PR, including screenshots of any UI changes:

After this commit, DbApiHook.run() will only commit when the autocommit field is set for the connection. For those don't support autocommit (e.g. sqlite_hook), the connection won't commit the query, which is a change of behavior.

This is currently breaking CI (tests/core.py:CoreTest.test_check_operators).

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    fixes tests/core.py:CoreTest.test_check_operators

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
    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.
    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

  • Passes git diff upstream/master -u -- "*.py" | flake8 --diff

@KevinYang21
Copy link
Member Author

KevinYang21 commented Jun 10, 2018

@imroc @Fokko @bolkedebruin @aoen PTAL, wanna get rid of those failed CI tests in my other PRs.

I'm only fixing the behavior change for db that does not support autocommit. But the PR I referred in the description had another change of behavior which is to change the default value of autocommit in the parameter. Would someone elaborate why do we want to change that please? I'm a bit worried about the performance implication from that change.

@KevinYang21 KevinYang21 changed the title [AIRFLOW-2590] Fix commit in DbApiHook.run() for no-autocommit DB [WIP][AIRFLOW-2590] Fix commit in DbApiHook.run() for no-autocommit DB Jun 10, 2018
@codecov-io
Copy link

codecov-io commented Jun 10, 2018 •
edited
Loading

Codecov Report

Merging #3485 into master will increase coverage by 59.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3485      +/-   ##
==========================================
+ Coverage   17.95%   77.16%   +59.2%     
==========================================
  Files         203      203              
  Lines       15124    15124              
==========================================
+ Hits         2716    11670    +8954     
+ Misses      12408     3454    -8954
Impacted Files Coverage Δ
airflow/hooks/dbapi_hook.py 82.35% <100%> (+58.82%) ⬆️
airflow/utils/operator_resources.py 86.95% <0%> (+4.34%) ⬆️
airflow/settings.py 80.5% <0%> (+5.08%) ⬆️
airflow/executors/__init__.py 63.46% <0%> (+5.76%) ⬆️
airflow/utils/decorators.py 91.66% <0%> (+14.58%) ⬆️
airflow/__init__.py 80.43% <0%> (+15.21%) ⬆️
airflow/task/task_runner/__init__.py 63.63% <0%> (+18.18%) ⬆️
airflow/utils/db.py 33.33% <0%> (+18.25%) ⬆️
airflow/macros/__init__.py 81.48% <0%> (+18.51%) ⬆️
airflow/operators/dummy_operator.py 100% <0%> (+22.22%) ⬆️
... and 157 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 c27098b...12f414a. Read the comment docs.

@KevinYang21 KevinYang21 changed the title [WIP][AIRFLOW-2590] Fix commit in DbApiHook.run() for no-autocommit DB [AIRFLOW-2590] Fix commit in DbApiHook.run() for no-autocommit DB Jun 10, 2018
@imroc
Copy link
Contributor

imroc commented Jun 11, 2018

@yrqls21 you're all right, the default value of autocommit should be False, my bad, I changed it back to False in this PR

@KevinYang21
Copy link
Member Author

KevinYang21 commented Jun 11, 2018 •
edited
Loading

@saguziel PTAL, wanna unblock the CI ASAP

@gerardo gerardo mentioned this pull request Jun 11, 2018
Merged
12 tasks
kaxil
kaxil approved these changes Jun 12, 2018
View reviewed changes
Copy link
Member

@kaxil kaxil left a comment

Choose a reason for hiding this comment

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

LGTM

@kaxil
Copy link
Member

kaxil commented Jun 12, 2018

Thanks for fixing the CI

@asfgit asfgit closed this in dae8ac3 Jun 12, 2018
@KevinYang21 KevinYang21 mentioned this pull request Jun 12, 2018
Closed
6 tasks
aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
@KevinYang21
[AIRFLOW-2590] Fix commit in DbApiHook.run() for no-autocommit DB
bbc7534 
Closes apache#3485 from yrqls21/kevin_yang_fix_dbapi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kaxil kaxil kaxil approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@KevinYang21 @codecov-io @imroc @kaxil