dstandish commented on a change in pull request #6370: AIRFLOW-5701: Don't
clear xcom explicitly before execution
URL: https://github.com/apache/airflow/pull/6370#discussion_r339308863
##########
File path: airflow/models/xcom.py
##########
@@ -43,16 +43,14 @@ class XCom(Base, LoggingMixin):
"""
__tablename__ = "xcom"
- id = Column(Integer, primary_key=True)
- key = Column(String(512))
+ key = Column(String(512), primary_key=True, nullable=False)
value = Column(LargeBinary)
- timestamp = Column(
- UtcDateTime, default=timezone.utcnow, nullable=False)
- execution_date = Column(UtcDateTime, nullable=False)
+ timestamp = Column(UtcDateTime, default=timezone.utcnow, nullable=False)
+ execution_date = Column(UtcDateTime, primary_key=True, nullable=False)
# source information
- task_id = Column(String(ID_LEN), nullable=False)
- dag_id = Column(String(ID_LEN), nullable=False)
+ task_id = Column(String(ID_LEN), primary_key=True, nullable=False)
+ dag_id = Column(String(ID_LEN), primary_key=True, nullable=False)
__table_args__ = (
Index('idx_xcom_dag_task_date', dag_id, task_id, execution_date,
unique=False),
Review comment:
I was mainly looking at this PR mainly to learn how to do DB change, in case
I ever want to propose something involving DB schema change. But noticed these
things and wanted to surface in case something was possibly missed.
**Clarification of observations**
* I noticed that only migration changes in this PR are in the
`current_schema` migration script, and they are only applied if table not
exists. So this indicated to me that this change would not be applied on
upgrades, whether sqlite or any other database.
* I tried doing a new install (on sqlite) and both index and primary key
were present.
* I tried doing upgrade (on sqlite) and the `id` column was not dropped.
**sqlite migration issues**
Re sqlite migrations, [this stack overflow
post](https://stackoverflow.com/a/32510603/2761682) that indicates that table
alters on sqlite are supported with alembic > 0.7.0 using batch mode, where it
will handle creating new tables and copying data. Wondering if that is
possibly relevant here.
**alembic revision issues**
I tried running `alembic revision` locally and encountered 2 issues. 1 was
typing import resolution error due to new `airflow/typing.py` module being in
same directory where we try to run alembic. 2 was `FAILED: Multiple heads are
present;` in alembic. Are migrations are handled by release manager so we
don't deal with them in PRs?
This [blog
post](https://blog.jerrycodes.com/multiple-heads-in-alembic-migrations/)
suggests adding unit test for detecting multiple revision heads. Maybe that's
a good idea.
**upgrades**
are we meant to be able to upgrade to 2.0 from 1.10.X? cus i tried
installing fresh install of 1.10.6 and then checking out master and running
`airflow db upgrade` and got an error. Is this something that we should be
testing for possibly?
Sorry, know, these questions sort of venture off out of scope of 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:
[email protected]
With regards,
Apache Git Services
