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 2020/12/23 04:53:06 UTC
[GitHub] [airflow] houqp opened a new pull request #13269: fix serialize_dag schema migration data column type
houqp opened a new pull request #13269:
URL: https://github.com/apache/airflow/pull/13269
text column in mysql is too small for large DAGs, resulting in invalid json blob being stored.
----------------------------------------------------------------
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 #13269: fix serialize_dag schema migration data column type
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13269:
URL: https://github.com/apache/airflow/pull/13269#issuecomment-831608996
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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] potiuk commented on pull request #13269: fix serialize_dag schema migration data column type
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13269:
URL: https://github.com/apache/airflow/pull/13269#issuecomment-749941714
Should not we add another migration for that? I think the jinni is out of the bottle now, and in order to fix it for someone who already run the migration, we have to add a new migration.
----------------------------------------------------------------
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] houqp commented on pull request #13269: fix serialize_dag schema migration data column type
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #13269:
URL: https://github.com/apache/airflow/pull/13269#issuecomment-781802795
After thinking more about this, we should just drop this change because in Airflow 2.x, all these dags will be stored as json type in MySQL (>5.6) or Postgres. So perhaps what we should do instead is to add a check in airflow-upgrade-check to see if existing column has been converted to json?
----------------------------------------------------------------
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 #13269: fix serialize_dag schema migration data column type
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13269:
URL: https://github.com/apache/airflow/pull/13269#discussion_r570174981
##########
File path: airflow/migrations/versions/d38e04c12aa2_add_serialized_dag_table.py
##########
@@ -45,7 +45,7 @@ def upgrade():
try:
conn.execute("SELECT JSON_VALID(1)").fetchone()
except (sa.exc.OperationalError, sa.exc.ProgrammingError):
- json_type = sa.Text
+ json_type = sa.LargeBinary
Review comment:
Why not LargeText? (I.e. and this col be binary or text?)
----------------------------------------------------------------
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] closed pull request #13269: fix serialize_dag schema migration data column type
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #13269:
URL: https://github.com/apache/airflow/pull/13269
--
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 pull request #13269: fix serialize_dag schema migration data column type
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #13269:
URL: https://github.com/apache/airflow/pull/13269#issuecomment-801813860
@houqp Should we close this PR then?
----------------------------------------------------------------
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 edited a comment on pull request #13269: fix serialize_dag schema migration data column type
Posted by GitBox <gi...@apache.org>.
ashb edited a comment on pull request #13269:
URL: https://github.com/apache/airflow/pull/13269#issuecomment-782017980
> After thinking more about this, we should just drop this change because in Airflow 2.x, all these dags will be stored as json type in MySQL (>5.6) or Postgres. So perhaps what we should do instead is to add a check in airflow-upgrade-check to see if existing column has been converted to json?
What would have converted that type to JSON?
(Users shouldn't ever have to manually edit the metadata db -- that's what migrations are for)
----------------------------------------------------------------
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 #13269: fix serialize_dag schema migration data column type
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13269:
URL: https://github.com/apache/airflow/pull/13269#issuecomment-749942379
[The Workflow run](https://github.com/apache/airflow/actions/runs/439608511) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^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] ashb commented on pull request #13269: fix serialize_dag schema migration data column type
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #13269:
URL: https://github.com/apache/airflow/pull/13269#issuecomment-782017980
> After thinking more about this, we should just drop this change because in Airflow 2.x, all these dags will be stored as json type in MySQL (>5.6) or Postgres. So perhaps what we should do instead is to add a check in airflow-upgrade-check to see if existing column has been converted to json?
What would have converted that type to JSON?
----------------------------------------------------------------
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] potiuk edited a comment on pull request #13269: fix serialize_dag schema migration data column type
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #13269:
URL: https://github.com/apache/airflow/pull/13269#issuecomment-749941714
Should not we add another migration for that? I think the gennie is out of the bottle now, and in order to fix it for someone who already run the migration, we have to add a new migration.
----------------------------------------------------------------
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 #13269: fix serialize_dag schema migration data column type
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13269:
URL: https://github.com/apache/airflow/pull/13269#discussion_r570174981
##########
File path: airflow/migrations/versions/d38e04c12aa2_add_serialized_dag_table.py
##########
@@ -45,7 +45,7 @@ def upgrade():
try:
conn.execute("SELECT JSON_VALID(1)").fetchone()
except (sa.exc.OperationalError, sa.exc.ProgrammingError):
- json_type = sa.Text
+ json_type = sa.LargeBinary
Review comment:
Why not LargeText? (I.e. and this col be binary or text?)
----------------------------------------------------------------
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] kaxil commented on pull request #13269: fix serialize_dag schema migration data column type
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #13269:
URL: https://github.com/apache/airflow/pull/13269#issuecomment-750443316
Yup we will need to add another migration for this.
Should we make the Column Type consistent with https://github.com/apache/airflow/commit/f66a46db88da86b4a11c5ee142c09a5001c32c41#diff-fd7ce54d2146064dd83398f84ab648a0ffad06e941a76c53c82901257204f779 ?
----------------------------------------------------------------
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