You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/03/18 05:58:56 UTC
[GitHub] [airflow] ericpp opened a new pull request #14869: Fixed autocommit calls for mysql-connector-python
ericpp opened a new pull request #14869:
URL: https://github.com/apache/airflow/pull/14869
The MySQLdb and mysql-connector-python clients use different methods for getting and setting the autocommit mode. This code checks the `conn` param passed into get/set_autocommit() to see if it's a MySQLdb instance (containing the get_autocommit method) or a mysql-connector-python instance (missing get_autocommit) and makes the appropriate autocommit calls for the client.
Fixes #14857
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#issuecomment-809041552
Awesome work, congrats on your first merged pull request!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r602436192
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
Review comment:
ok i can try to look later today
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r597203702
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
"""MySql connection sets autocommit in a different way."""
- conn.autocommit(autocommit)
+ if hasattr(conn, 'get_autocommit'): # mysqlclient
+ conn.autocommit(autocommit)
+ else: # mysql-connector-python
+ conn.autocommit = autocommit
Review comment:
This was a tricky one!
I totally agree we should avoid generating the side effect twice.
Here are two options I came up with that I think are a little more direct.
First option is you can check the class of `conn`.
I am pretty sure that mysql connector will always inherit from this base class:
if isinstance(conn, MySQLConnectionAbstract):
conn.autocommit = autocommit
else:
conn.autocommit(autocommit)
Though alternatively you could check if it's MySQLdb:
```
from MySQLdb.connections import Connection as MySQLdbConnection
...
if isinstance(conn, MySQLdbConnection):
conn.autocommit(autocommit)
else:
conn.autocommit = autocommit
```
And the import alias of MySQLdbConnection can be re-used in resolving that type annotation problem. I'm not sure what's the best practice re aliasing classes in the event of a collision like we have here but this approach seems reasonable to me.
Option 2 (and I think I like this one) is check if `autocommit` is a property. You can do so without calling it like so:
```python
if isinstance(conn.__class__.autocommit, property):
conn.autocommit = autocommit
else:
conn.autocommit(autocommit)
```
I think _also_ checking whether that property has a setter is overkill and not worth the complexity but you could do so with `hasattr(conn.__class__.autocommit, 'fset')`.
But I think I'd go with either of these two approaches because checking for `get_autocommit` and not using it is a little confusing and certainly indirect.
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r597581136
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
Review comment:
Ash provided another option in slack ....
You could put the annotation import behind `if TYPING:`
I think either way is ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r601646361
##########
File path: tests/providers/mysql/hooks/test_mysql.py
##########
@@ -184,6 +197,20 @@ def setUp(self):
self.db_hook.get_connection = mock.Mock()
self.db_hook.get_connection.return_value = self.connection
+ class FakeMySQLConnection(object):
Review comment:
I've reworked this to have the fake object extend the `mock.MagicMock` class so that the `mysq.py` property checks pass and so that the mock assertions can be used.
##########
File path: tests/providers/mysql/hooks/test_mysql.py
##########
@@ -184,6 +197,20 @@ def setUp(self):
self.db_hook.get_connection = mock.Mock()
self.db_hook.get_connection.return_value = self.connection
+ class FakeMySQLConnection(object):
Review comment:
I've reworked this to have the fake object extend the `mock.MagicMock` class so that the `mysql.py` property checks pass and so that the mock assertions can be used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r597965321
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
Review comment:
btw i explored this idea...
this seems to at least run (and it accomlishes the goal of providing correct type annotation without importing...
not sure how mypy will feel about it but you could try it
and not sure if it is exactly what ash is suggesting.
```python
TYPING = False
if TYPING:
from MySQLdb.connections import Connection as MySQLdbConnection
from mysql.connector.abstracts import MySQLConnectionAbstract
...
def set_autocommit(
self, conn: Union['MySQLdbConnection', 'MySQLConnectionAbstract'], autocommit: bool
) -> None:
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r602418810
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
Review comment:
Added in recent commits, but not sure how to actually test it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r597203702
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
"""MySql connection sets autocommit in a different way."""
- conn.autocommit(autocommit)
+ if hasattr(conn, 'get_autocommit'): # mysqlclient
+ conn.autocommit(autocommit)
+ else: # mysql-connector-python
+ conn.autocommit = autocommit
Review comment:
This was a tricky one!
I totally agree we should avoid generating the side effect twice.
Here are two options I came up with that I think are a little more direct.
First option is you can check the class of `conn`.
I am pretty sure that mysql connector will always inherit from this base class:
if isinstance(conn, MySQLConnectionAbstract):
conn.autocommit = autocommit
else:
conn.autocommit(autocommit)
Though alternatively you could check if it's MySQLdb:
```
from MySQLdb.connections import Connection as MySQLdbConnection
...
if isinstance(conn, MySQLdbConnection):
conn.autocommit(autocommit)
else:
conn.autocommit = autocommit
```
And the import alias of MySQLdbConnection can be re-used in resolving that type annotation problem. I'm not sure what's the best practice re aliasing classes in the event of a collision like we have here but this approach seems reasonable to me.
Option 2 (and I think I like this one) is check if `autocommit` is a property. You can do so without calling it like so:
```python
if isinstance(conn.__class__.autocommit, property):
conn.autocommit = autocommit
else:
conn.autocommit(autocommit)
```
I think _also_ checking whether that property has a setter is overkill and certainly not worth the complexity, but in the course of looking into this I learned you can do so with `hasattr(conn.__class__.autocommit, 'fset')`.
But I think I'd go with either of these two approaches because checking for `get_autocommit` and not using it is a little confusing and certainly indirect.
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#issuecomment-808443751
> @dstandish Don't we need mysql.connector-specific tests for `test_run_with_autocommit` and ` test_run_without_autocommit` ? Should those be handled separately from this PR?
I think those tests are somewhat duplicative and non-specific. You have fixed a problem with the methods get and set autocommit, and now there are tests that verify that get and set autocommit work as expected. If there are other methods that call get and set, well, they can simply veryify that they called get and set autocommit -- they don't need to retest that get and set work.
So your fix here is tested, and it does not require duplication of all those other tests. And while they might benefit from a refactor, it's not necessary for this PR. So my take is, you are making and an incremental improvement here, and not making anything worse, without opening up any other hornets nests.
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp edited a comment on pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp edited a comment on pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#issuecomment-804206437
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r597203702
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
"""MySql connection sets autocommit in a different way."""
- conn.autocommit(autocommit)
+ if hasattr(conn, 'get_autocommit'): # mysqlclient
+ conn.autocommit(autocommit)
+ else: # mysql-connector-python
+ conn.autocommit = autocommit
Review comment:
This was a tricky one!
I totally agree we should avoid generating the side effect twice.
Here are two options I came up with that I think are a little more direct.
First option is you can check the class of `conn`.
I am pretty sure that mysql connector will always inherit from this base class:
```python
if isinstance(conn, MySQLConnectionAbstract):
conn.autocommit = autocommit
else:
conn.autocommit(autocommit)
```
Though alternatively you could check if it's MySQLdb:
```python
from MySQLdb.connections import Connection as MySQLdbConnection
...
if isinstance(conn, MySQLdbConnection):
conn.autocommit(autocommit)
else:
conn.autocommit = autocommit
```
And the import alias of MySQLdbConnection can be re-used in resolving that type annotation problem. I'm not sure what's the best practice re aliasing classes in the event of a collision like we have here but this approach seems reasonable to me.
Option 2 (and I think I like this one) is check if `autocommit` is a property. You can do so without calling it like so:
```python
if isinstance(conn.__class__.autocommit, property):
conn.autocommit = autocommit
else:
conn.autocommit(autocommit)
```
I think _also_ checking whether that property has a setter is overkill and certainly not worth the complexity, but in the course of looking into this I learned you can do so with `hasattr(conn.__class__.autocommit, 'fset')`.
But I think I'd go with either of these two approaches because checking for `get_autocommit` and not using it is a little confusing and certainly indirect.
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#issuecomment-801652352
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/master/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) for testing locally, itโs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better ๐.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#issuecomment-808395665
Would you like me to squash my commits when this is ready to be merged? I added a bunch of development commits which are probably not useful to have in the main codebase.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r602441645
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
Review comment:
@ericpp i know you were working through the right way to test this autocommit fix, but it seems you added a bunch of tests that are unrelated to this change? can you remove the tests that don't have anything to do with this PR? perhaps i'm missing something?
after you do that, i'll take a look and see if we need to make any adjustments, or if you have specific questions i can try to address them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r602473254
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
Review comment:
I just removed the `test_get_autocommit` test that I did add to the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r597581136
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
Review comment:
Ash provided another option in slack ....
You could put the annotation import behind `if TYPE_CHECKING:`
I think either way is ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#issuecomment-807238467
[The Workflow run](https://github.com/apache/airflow/actions/runs/687514890) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r601646361
##########
File path: tests/providers/mysql/hooks/test_mysql.py
##########
@@ -184,6 +197,20 @@ def setUp(self):
self.db_hook.get_connection = mock.Mock()
self.db_hook.get_connection.return_value = self.connection
+ class FakeMySQLConnection(object):
Review comment:
I've reworked this to have the fake object extend the `MagicMock` class so that the property checks in `mysql.py` pass and so that the mock assertions can be used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r597049286
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
"""MySql connection sets autocommit in a different way."""
- conn.autocommit(autocommit)
+ if hasattr(conn, 'get_autocommit'): # mysqlclient
+ conn.autocommit(autocommit)
+ else: # mysql-connector-python
+ conn.autocommit = autocommit
Review comment:
I avoided doing that because `conn.autocommit` is actually a method with a property decorator around it. It has a side effect of running a `SELECT @@session.autocommit` query whenever you try to access it: https://github.com/mysql/mysql-connector-python/blob/86cceb2bc914362e0990459002976c7e5a271e1d/lib/mysql/connector/connection_cext.py#L148
I also decided against the `callable` function as it seems to not exist in some versions of Python 3. I'm not sure which versions you officially support: https://docs.python.org/3/library/functions.html#callable .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r597432774
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
Review comment:
Do you have to import both modules before you can use their names for a `Union` typing? If so, I would need to figure out if having them both imported at the same time would cause any issues. It looks like the `get_conn()` method is only importing one module depending on the value of `client_name`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#issuecomment-808411567
ok @ericpp i took a crack at writing tests for the autocommit behavior
i removed the unrelated changes. if you want to do a more comprehensive refactor of mysql hook tests i think it's better that you make a separate PR.
doing it minimally this way we can constrain our focus just to the task at hand
please take a look at my suggested approach and let me know what you think / make changes if appropriate
thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r596598632
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
"""MySql connection sets autocommit in a different way."""
- conn.autocommit(autocommit)
+ if hasattr(conn, 'get_autocommit'): # mysqlclient
+ conn.autocommit(autocommit)
+ else: # mysql-connector-python
+ conn.autocommit = autocommit
Review comment:
it's a little awkward to check for attr `get_autocommit` and then not use that attr but rather to only use it as a mechanism to infer that the `conn` variable is mysqldb and therefore `autocommit` must be callable
better would be to just check if autocommit is callable directly:
```suggestion
if callable(conn.autocommit):
conn.autocommit(autocommit)
else:
conn.autocommit = autocommit
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r597051472
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
"""MySql connection sets autocommit in a different way."""
- conn.autocommit(autocommit)
+ if hasattr(conn, 'get_autocommit'): # mysqlclient
+ conn.autocommit(autocommit)
+ else: # mysql-connector-python
+ conn.autocommit = autocommit
Review comment:
All good points lemme take another look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r602418206
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
"""MySql connection sets autocommit in a different way."""
- conn.autocommit(autocommit)
+ if hasattr(conn, 'get_autocommit'): # mysqlclient
+ conn.autocommit(autocommit)
+ else: # mysql-connector-python
+ conn.autocommit = autocommit
def get_autocommit(self, conn: Connection) -> bool: # noqa: D403
Review comment:
Moved to docblock
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#issuecomment-804324195
@dstandish I've changed the PR to draft and pushed out the tests I have so far. I still need to recreate the run with/without autocommit tests for each client.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#issuecomment-808436363
@dstandish Don't we need mysql.connector-specific tests for `test_run_with_autocommit` and ` test_run_without_autocommit` ? Or should those be handled separate from this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r599019607
##########
File path: tests/providers/mysql/hooks/test_mysql.py
##########
@@ -184,6 +197,20 @@ def setUp(self):
self.db_hook.get_connection = mock.Mock()
self.db_hook.get_connection.return_value = self.connection
+ class FakeMySQLConnection(object):
Review comment:
since airflow is python 3+ you can remove `(object)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r597376966
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -63,7 +66,10 @@ def get_autocommit(self, conn: Connection) -> bool: # noqa: D403
:return: connection autocommit setting
:rtype: bool
"""
- return conn.get_autocommit()
+ if hasattr(conn, 'get_autocommit'): # mysqlclient
Review comment:
It looks like those are the package names whereas `MySQLdb` and `mysql.connector` are the module names:
https://pypi.org/project/mysqlclient/
https://pypi.org/project/mysql-connector-python/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r596598632
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
"""MySql connection sets autocommit in a different way."""
- conn.autocommit(autocommit)
+ if hasattr(conn, 'get_autocommit'): # mysqlclient
+ conn.autocommit(autocommit)
+ else: # mysql-connector-python
+ conn.autocommit = autocommit
Review comment:
it's a little awkward to check for attr `get_autocommit` and then not use that attr ... but rather to only use it as a mechanism to infer that `autocommit` is callable
better would be to just check if autocommit is callable directly:
```suggestion
if callable(conn.autocommit):
conn.autocommit(autocommit)
else:
conn.autocommit = autocommit
```
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
Review comment:
in the code here, presently this `Connection` refers to the airflow model connection but this is not correct
it should be this one: `from MySQLdb.connections import Connection`
now might be a good opportunity to fix this, and also make it a `Union` with the mysql-connector connection type also
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -63,7 +66,10 @@ def get_autocommit(self, conn: Connection) -> bool: # noqa: D403
:return: connection autocommit setting
:rtype: bool
"""
- return conn.get_autocommit()
+ if hasattr(conn, 'get_autocommit'): # mysqlclient
Review comment:
the comment says `mysqlclient ` but isn't the library actually called `MySQLdb`? not sure you really need the comments but either way is fine
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
"""MySql connection sets autocommit in a different way."""
- conn.autocommit(autocommit)
+ if hasattr(conn, 'get_autocommit'): # mysqlclient
+ conn.autocommit(autocommit)
+ else: # mysql-connector-python
+ conn.autocommit = autocommit
def get_autocommit(self, conn: Connection) -> bool: # noqa: D403
Review comment:
here too i'd suggest fixing this type annotation while you're at it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp edited a comment on pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp edited a comment on pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#issuecomment-808436363
@dstandish Don't we need mysql.connector-specific tests for `test_run_with_autocommit` and ` test_run_without_autocommit` ? Should those be handled separately from this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r602458737
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
Review comment:
oh i see thanks lemme look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r597437118
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
Review comment:
again this is a good point.
the `mysql` extra installs both connector python and mysqldb so it should be safe to import both. i don't think we save much by importing locally.
i'll ask around though. please write a test for this also -- it is testable behavior and i think merits it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r597051840
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -63,7 +66,10 @@ def get_autocommit(self, conn: Connection) -> bool: # noqa: D403
:return: connection autocommit setting
:rtype: bool
"""
- return conn.get_autocommit()
+ if hasattr(conn, 'get_autocommit'): # mysqlclient
Review comment:
I got those names from the `client_name` variables in the get_conn() method. I agree it makes more sense to use the names of the actual libraries rather than the `client_name`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#issuecomment-804334713
> @dstandish I've changed the PR to draft and pushed out the tests I have so far. I still need to recreate the run with/without autocommit tests for each client.
great thank you :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r597567433
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
Review comment:
Ok, given that, I guess we should just remove that type annotation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r597965321
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
Review comment:
btw i explored this idea...
```python
from typing import TYPE_CHECKING
if TYPE_CHECKING:
from mysql.connector.abstracts import MySQLConnectionAbstract
from MySQLdb.connections import Connection as MySQLdbConnection
...
def set_autocommit(
self, conn: Union['MySQLdbConnection', 'MySQLConnectionAbstract'], autocommit: bool
) -> None:
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r597566030
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
Review comment:
Generally: one of the biggest slow downs in Airflow "boot" time is importing modules, so avoiding top level imports where it's not strictly needed is better I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#issuecomment-808463102
@dstandish That makes sense to me. It does seem strange for those methods to access the connection autocommit methods directly rather than through the get/set methods in the hook.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r597376439
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
"""MySql connection sets autocommit in a different way."""
- conn.autocommit(autocommit)
+ if hasattr(conn, 'get_autocommit'): # mysqlclient
+ conn.autocommit(autocommit)
+ else: # mysql-connector-python
+ conn.autocommit = autocommit
Review comment:
Thanks! I like option two because it directly tests the thing it's trying to use.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r602459790
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
Review comment:
> I moved the code that works for both clients from TestMySqlHook into TestMySqlBulk
ok maybe not 'moved' but 'copied'
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r601701317
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -50,20 +56,36 @@ def __init__(self, *args, **kwargs) -> None:
self.schema = kwargs.pop("schema", None)
self.connection = kwargs.pop("connection", None)
- def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
- """MySql connection sets autocommit in a different way."""
- conn.autocommit(autocommit)
+ def set_autocommit(self, conn: MySQLConnectionTypes, autocommit: bool) -> None:
+ """
+ The MySQLdb (mysqlclient) client uses an `autocommit` method rather
+ than an `autocommit` property to set the autocommit setting
+
+ :param conn: connection to set autocommit setting
+ :type MySQLConnectionTypes: connection object.
+ :param autocommit: autocommit setting
+ :type bool: True to enable autocommit, False to disable autocommit
+ :rtype: None
+ """
+ if hasattr(conn.__class__, 'autocommit') and isinstance(conn.__class__.autocommit, property):
+ conn.autocommit = autocommit
+ else:
+ conn.autocommit(autocommit)
Review comment:
I avoided that because it has the side effect of running a `SELECT @@session.autocommit` query whenever the `autocommit` property is accessed in the `mysql-connector-python` client
See: https://github.com/apache/airflow/pull/14869#discussion_r596598632
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r597437118
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
Review comment:
again this is a good point.
the `mysql` extra installs both connector python and mysqldb so it should be safe to import both. i don't think we save much by importing locally.
i'll ask around though. please write a test for this also -- it (your autocommit fix) is testable behavior and i think merits it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#issuecomment-808469194
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r602418397
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -63,7 +66,10 @@ def get_autocommit(self, conn: Connection) -> bool: # noqa: D403
:return: connection autocommit setting
:rtype: bool
"""
- return conn.get_autocommit()
+ if hasattr(conn, 'get_autocommit'): # mysqlclient
Review comment:
Moved to docblock
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r601646361
##########
File path: tests/providers/mysql/hooks/test_mysql.py
##########
@@ -184,6 +197,20 @@ def setUp(self):
self.db_hook.get_connection = mock.Mock()
self.db_hook.get_connection.return_value = self.connection
+ class FakeMySQLConnection(object):
Review comment:
I've reworked this to have the fake object extend the `mock.MagicMock` class so that the property checks in `mysql.py` pass and so that the mock assertions can be used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#issuecomment-804206437
> @ericpp looks like the test needs to be updated:
>
> ```python
> def set_autocommit(self, conn: MySQLConnectionTypes, autocommit: bool) -> None:
> """MySql connection sets autocommit in a different way."""
> > if isinstance(conn.__class__.autocommit, property): # mysql-connector-python
> E AttributeError: type object 'MagicMock' has no attribute 'autocommit'
> ```
Thanks. I'm still working on the tests. I'm having troubles getting that `isinstance` check to pass for the any of the mocking methods. I did get it to work by creating a custom class that has its own defined `autocommit` decorated property methods, but that class has no connection back to the `mysql-connector-python` class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish merged pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish merged pull request #14869:
URL: https://github.com/apache/airflow/pull/14869
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#issuecomment-804200824
@ericpp looks like the test needs to be updated:
```python
def set_autocommit(self, conn: MySQLConnectionTypes, autocommit: bool) -> None:
"""MySql connection sets autocommit in a different way."""
> if isinstance(conn.__class__.autocommit, property): # mysql-connector-python
E AttributeError: type object 'MagicMock' has no attribute 'autocommit'
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r602455284
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
Review comment:
The `TestMySqlHookMySqlConnectorPython` class is a version of the existing `TestMySqlHook` class that works for the `mysql-connector-python` client. It seems like the existing `TestMySqlHook` class only supports the `MySQLdb` client.
I moved the code that works for both clients from `TestMySqlHook` into `TestMySqlBulk`
I did add a `test_get_autocommit` method to both test classes and can remove that if needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#issuecomment-808406376
> Would you like me to squash my commits when this is ready to be merged? I added a bunch of development commits which are probably not useful to have in the main codebase.
don't worry about it we always squash before merge
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ericpp commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
ericpp commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r602467654
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
Review comment:
The test_bulk methods were moved into the `TestMySqlBulk` class. The git diff looks strange because it plopped my changes into middle of the old `TestMySql` class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r601690218
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -50,20 +56,36 @@ def __init__(self, *args, **kwargs) -> None:
self.schema = kwargs.pop("schema", None)
self.connection = kwargs.pop("connection", None)
- def set_autocommit(self, conn: Connection, autocommit: bool) -> None: # noqa: D403
- """MySql connection sets autocommit in a different way."""
- conn.autocommit(autocommit)
+ def set_autocommit(self, conn: MySQLConnectionTypes, autocommit: bool) -> None:
+ """
+ The MySQLdb (mysqlclient) client uses an `autocommit` method rather
+ than an `autocommit` property to set the autocommit setting
+
+ :param conn: connection to set autocommit setting
+ :type MySQLConnectionTypes: connection object.
+ :param autocommit: autocommit setting
+ :type bool: True to enable autocommit, False to disable autocommit
+ :rtype: None
+ """
+ if hasattr(conn.__class__, 'autocommit') and isinstance(conn.__class__.autocommit, property):
+ conn.autocommit = autocommit
+ else:
+ conn.autocommit(autocommit)
Review comment:
Would this be simpler?
```suggestion
if callable(conn.autocommit):
conn.autocommit(autocommit)
else:
conn.autocommit = autocommit
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #14869: Fixed autocommit calls for mysql-connector-python
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#issuecomment-808469694
@feluelle would you mind taking a quick look and let us know if you're good with this approach? just want second set of eyes and you have some experience with this hook.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org