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 in that `current_schema` migration script, changes are only
applied if table not exists. So this suggested 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
remain.
* 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.
----------------------------------------------------------------
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
