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

Mysql change xcom value col type for MySQL backend #38401

Merged
merged 4 commits into from Mar 23, 2024

Conversation

pankajastro
Copy link
Member

@pankajastro pankajastro commented Mar 22, 2024

Xcom value we can store Airflow metadata is a bit smaller
when we use MySQL as a database backend since by default,
the Sqlalchemy map LargeBinary to MySQL blob type
which can store up to 65,535 bytes. In this PR,
I'm proposing to change the value column type to
longblob for xcom table if a user using MySQL database backend
https://dev.mysql.com/doc/refman/8.0/en/string-type-syntax.html


^ 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.

@pankajastro pankajastro changed the title Mysql change xcom value type Mysql change xcom value type for MySQL backend Mar 22, 2024
@pankajastro pankajastro force-pushed the mysql_change_xcom_value_type branch 2 times, most recently from 3ea0608 to 09f0ca2 Compare March 22, 2024 12:15
@pankajastro pankajastro changed the title Mysql change xcom value type for MySQL backend Mysql change xcom value col type for MySQL backend Mar 22, 2024
@pankajastro pankajastro marked this pull request as ready for review March 22, 2024 13:04
@potiuk
Copy link
Member

potiuk commented Mar 22, 2024

Will that cause a long migration process possibly ? or will such migration be rather fast ? I suspect if there is something to rewrite the way how the blobs are stored, it might take an awfully long time to run such a migration.

I think, if that's the case then we will likely have to at least warn the users in significant note that it might happen and that they should likely run db clean on xcom table for some old data?

@pankajastro
Copy link
Member Author

Will that cause a long migration process possibly ?

Yes, that will be the case if they have lots of large data in Xcom, although I don't have a specific number at the moment.

I think, if that's the case then we will likely have to at least warn the users in significant note that it might happen and that they should likely run db clean on xcom table for some old data?

This makes sense. I'll add a warning.
If the blob size > 65,535 bytes then to downgrade they will have to clean at least large data.

@pankajastro
Copy link
Member Author

@potiuk I have tried to add a note docs/apache-airflow/howto/set-up-database.rst but not sure if this right place. any suggestions?

@potiuk
Copy link
Member

potiuk commented Mar 22, 2024

Should be a significant newsfragment too.

Copy link
Member

@jedcunningham jedcunningham left a comment

Choose a reason for hiding this comment

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

We should definitely test whether this will rewrite all of xcom - it might not.

If it does, and it results in a slow migration, I really wonder if we should not do this and instruct users to set up an object storage xcom backend instead. Thoughts?

docs/apache-airflow/howto/set-up-database.rst Outdated Show resolved Hide resolved
@Taragolis
Copy link
Contributor

I think, if that's the case then we will likely have to at least warn the users in significant note that it might happen and that they should likely run db clean on xcom table for some old data?

A bit Friday jokes: If take in account last survey - number of MySQL users about 11-14 % (11% MySQL 8, and 3.4% MySQL 5) for avoid long migration it could be an option to start from scratch with Postgres 🤣 .

@potiuk
Copy link
Member

potiuk commented Mar 22, 2024

I think, if that's the case then we will likely have to at least warn the users in significant note that it might happen and that they should likely run db clean on xcom table for some old data?

A bit Friday jokes: If take in account last survey - number of MySQL users about 11-14 % (11% MySQL 8, and 3.4% MySQL 5) for avoid long migration it could be an option to start from scratch with Postgres 🤣 .

Yes. Let's also add "if mysql: sleep(0.2)" in various places in Airlfow. Enough to be impacting performance, but not enough to say "MYSQL does not work". Then we will be able to tell "But Postgres is WAY faster".

@kaxil
Copy link
Member

kaxil commented Mar 22, 2024

If it does, and it results in a slow migration, I really wonder if we should not do this and instruct users to set up an object storage xcom backend instead. Thoughts?

The current size is low per row - 64kb for MySQL, so increasing to 64 MB should be supported without Custom XCom backend

@pankajastro pankajastro merged commit 078e7ed into apache:main Mar 23, 2024
46 checks passed
@pankajastro pankajastro deleted the mysql_change_xcom_value_type branch March 23, 2024 13:19
@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label Mar 23, 2024
@kaxil kaxil added this to the Airflow 2.9.0 milestone Mar 24, 2024
utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
* Change table Xcom value column value type to longblob from blob

Xcom value we can store Airflow metadata is a bit smaller
when we use MySQL as a database backend since by default,
the Sqlalchemy map LargeBinary to MySQL blob type
which can store up to 65,535 bytes. In this PR,
I'm proposing to change the value column type to
longblob for xcom table if a user using MySQL database backend
https://dev.mysql.com/doc/refman/8.0/en/string-type-syntax.html
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

7 participants