[AIRFLOW-2597] Restore original dbapi.run() behavior

[KevinYang21]

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
  dbapi.run() right now commits only when autocommit is set to true or db does not support autocommit. Restoring behavior before this commit.
  This is breaking CI now.

Tests

• My PR adds the following unit tests OR does not need testing for this extremely good reason:
  fixes tests/operators/operators.py:PostgresTest.test_postgres_to_postgres

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]

I was misunderstanding what should_commit was for and thus this change doesn't completely fix the failing CI (not sure why the CI passed in the PR tho cuz this PR was created afterwards and merged before my PR was merged).

@imroc, I'm basically reverting your change, I don't see why it was not working for you before your change. Can you elaborate please? Thank you

@Fokko @bolkedebruin @artwr @kaxil @feng-tao PTAL

[codecov-io]

Codecov Report

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

@@            Coverage Diff             @@
##           master    #3490      +/-   ##
==========================================
+ Coverage   17.95%   77.16%   +59.2%     
==========================================
  Files         203      203              
  Lines       15124    15123       -1     
==========================================
+ Hits         2716    11669    +8953     
+ Misses      12408     3454    -8954

Impacted Files	Coverage Δ
airflow/hooks/dbapi_hook.py	82.2% <100%> (+58.67%)	⬆️
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 dae8ac3...90a1c52. Read the comment docs.

[feng-tao]

I think we should be very careful on any backward incompatible change and document it in the updating.md with unit test if it is really needed. I am +1 on the revert.

[kaxil]

Hmm.. Weird why the CI passed for that PR then... But yes, I am reverting the change. @imroc Please elaborate your issue here.

[KevinYang21]

@kaxil I edited my comment earlier, the PR you previously helped me merged passed the CI because this PR was created afterwards and merged before my PR was merged.

[kaxil]

Oh ok 👍 @yrqls21 . @Fokko Can you please have a look if the failing K8s test is related or not? That's the only test that is failing on master now.

[imroc]

@kaxil @yrqls21 I wrote a DAG before, including some data insertion operation of mysql like this:

hook = MySqlHook(mysql_conn_id='mysql_default', schema='test')

hook.run(
    'insert into some_table(some_field) values(%s)' % (some_value),
    autocommit=True)

After the task runs, data is not inserted, because it was not committed, But I set autocommit=True, I think it was a bug, so I submitted this commit，and then it works, is it wrong with my usage?

[feng-tao]

@imroc , I think typically you should run session.commit() after the sql execution to persist the result. Besides you change the dbapi interface which inherits by most of the dbapi hooks which not all the hooks have autocommit support.

[KevinYang21]

@imroc You are correct, the query won't be committed for MySqlHook.run() even autocommit is set to True. Did some researches an I'm pretty sure it is because MySql set autocommit in a different way. It should be conn.autocommit(True) instead of conn.autocommit = True like others.

I shall do a quick PR to fix that. ETA tomorrow night.

[imroc]

@yrqls21 nice