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

Fix db clean command for mysql db #29999

Merged
merged 6 commits into from
Mar 13, 2023

Conversation

tanvn
Copy link
Contributor

@tanvn tanvn commented Mar 9, 2023

closes: #28051

Tested on my local environment (running with MySQL 8) and confirmed that it is working well.
This is my very first PR, so if there is anything else I should do (like adding new tests?), please let me know.

@boring-cyborg
Copy link

boring-cyborg bot commented Mar 9, 2023

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/main/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits 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 Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment 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 for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    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

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.

LGTM. @dstandish = WDYT?

@potiuk
Copy link
Member

potiuk commented Mar 12, 2023

One NIT I'd add @tanvn is to add similar comment here as the one you referred in the issue:

        # MySQL with replication needs this split in to two queries, so just do it for all MySQL
        # ERROR 1786 (HY000): Statement violates GTID consistency: CREATE TABLE ... SELECT.

so that we know where it came from

bind = session.get_bind()
dialect_name = bind.dialect.name
if dialect_name == "mysql":
session.execute(f"CREATE TABLE {target_table_name} LIKE {orm_model.name}")
Copy link
Member

@potiuk potiuk Mar 12, 2023

Choose a reason for hiding this comment

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

Suggested change
session.execute(f"CREATE TABLE {target_table_name} LIKE {orm_model.name}")
# MySQL with replication needs this split in to two queries, so just do it for all MySQL
# ERROR 1786 (HY000): Statement violates GTID consistency: CREATE TABLE ... SELECT.
session.execute(f"CREATE TABLE {target_table_name} LIKE {orm_model.name}")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I added the comment!

@eladkal eladkal added this to the Airflow 2.5.3 milestone Mar 12, 2023
@eladkal eladkal added the type:bug-fix Changelog: Bug Fixes label Mar 12, 2023
if dialect_name == "mysql":
# MySQL with replication needs this split into two queries, so just do it for all MySQL
# ERROR 1786 (HY000): Statement violates GTID consistency: CREATE TABLE ... SELECT.
session.execute(f"CREATE TABLE {target_table_name} LIKE {orm_model.name}")
Copy link
Member

Choose a reason for hiding this comment

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

Instead of writing custom SQL here, would it be better to enhance CreateTableAs compilation to support the MySQL syntax instead?

Copy link
Contributor Author

@tanvn tanvn Mar 13, 2023

Choose a reason for hiding this comment

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

@uranusjr I would love to do so but I am new to sqlalchemy and at first I do not know how to let the compiler execute 2 queries instead of one?
(I just took a quick look at https://docs.sqlalchemy.org/en/20/core/compiler.html#dialect-specific-compilation-rules)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm good point. It’s probably possible with some SQL but likely not worthwhile.

Copy link
Contributor

@huymq1710 huymq1710 left a comment

Choose a reason for hiding this comment

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

LGTM

@potiuk potiuk merged commit 78cc2e8 into apache:main Mar 13, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Mar 13, 2023

Awesome work, congrats on your first merged pull request!

pierrejeambrun pushed a commit that referenced this pull request Mar 23, 2023
pierrejeambrun pushed a commit that referenced this pull request Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

airflow db clean command does not work with MySQL with enforce_gtid_consistency enabled
5 participants