Skip to content
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.

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

Remove mysql-connector-python from recommended MySQL driver #34287

Merged

Conversation

Taragolis
Copy link
Contributor

At least from middle of 2019 we run tests only with mysqlclient driver, and I can't find what the initial reason to make mysql-connector-python supported driver for MySQL backend.

In additional in SQLAlchemy documentation mysql-connector-python listed (1.4, 2.0) as not tested in their CI with potential unresolved issues.

Personally I found this a yet another nice way to shoot in foot or cast wormhole. Maybe I wrong and we should:

  • Started test against mysql-connector-python so we could say that this actually a supported driver
  • Change original from text from "We also support" to something like "MySQL backend might also work with"

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

+1

But I prefer to have the opinions of other people before merging

@uranusjr
Copy link
Member

Not sure about using important here, a simple note seems enough to me. It may also be a good idea to mention that it’s possible to use mysql-connector-python but the combination is not actively maintained by Airflow.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Yeah. I think we should absolutely not use the word "supported" if we don't test it. What we could use is we can make a reference to "You can also attempt to use other drivers. Please consult the SQL Alchemy documentation "

I think it's a but "unfair" to just remove it but, generally moving people out to the place where they can start searching for solution is the best way how we can say "you can try it on your own - see there for details" - this way we delegate it out to SQLAlchemy to describe it "well-enough".

@potiuk
Copy link
Member

potiuk commented Sep 12, 2023

Yeah. And note shoudl be good-enough

@Taragolis
Copy link
Contributor Author

What we could use is we can make a reference to "You can also attempt to use other drivers. Please consult the SQL Alchemy documentation "

Hmm.. we've already have it for MySQL backend

If you want to use other drivers visit the `MySQL Dialect <https://docs.sqlalchemy.org/en/13/dialects/mysql.html>`__  in SQLAlchemy documentation for more information regarding download
and setup of the SqlAlchemy connection.

About note vs important vs some other. We have quite a few notes and warnings in Set up a Database Backend so that the reason why I choose this type of block. I don't have any preferences, and could turn into the note

Some quick info about 3 most popular MySQL's DBAPI2 drivers (non-async).

Tested in Airflow's CI Tested in SA's CI Required build package
mysqlclient Yes
mysql-connector-python No (since 8.1)
PyMySQL No, pure python implementation

@potiuk
Copy link
Member

potiuk commented Sep 12, 2023

What we could use is we can make a reference to "You can also attempt to use other drivers. Please consult the SQL Alchemy documentation "

Hmm.. we've already have it for MySQL backend

If you want to use other drivers visit the `MySQL Dialect <https://docs.sqlalchemy.org/en/13/dialects/mysql.html>`__  in SQLAlchemy documentation for more information regarding download
and setup of the SqlAlchemy connection.

Cool. Then I am good.

@potiuk
Copy link
Member

potiuk commented Sep 12, 2023

About note vs important vs some other. We have quite a few notes and warnings in Set up a Database Backend so that the reason why I choose this type of block. I don't have any preferences, and could turn into the note

Both are fine for me.

@vincbeck vincbeck merged commit 44831db into apache:main Oct 20, 2023
42 checks passed
@Taragolis Taragolis deleted the remove-mysql-connector-as-recommended branch October 20, 2023 20:00
@ephraimbuddy ephraimbuddy added the type:doc-only Changelog: Doc Only label Oct 27, 2023
ephraimbuddy pushed a commit that referenced this pull request Oct 29, 2023
ephraimbuddy pushed a commit that referenced this pull request Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants