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 2022/01/17 10:51:33 UTC
[GitHub] [airflow] uranusjr opened a new pull request #20902: Switch map_index back to use server_default
uranusjr opened a new pull request #20902:
URL: https://github.com/apache/airflow/pull/20902
This ends up being simply reverting fff3a1e866bfc2d00d6454a5feb72203a8ce7513, but I forgot exactly why I made the switch in the first place. I guess the test suite will tell us if it works…
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #20902: Switch map_index back to use server_default
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20902:
URL: https://github.com/apache/airflow/pull/20902#issuecomment-1014621580
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #20902: Switch map_index back to use server_default
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20902:
URL: https://github.com/apache/airflow/pull/20902#discussion_r786066324
##########
File path: airflow/models/taskinstance.py
##########
@@ -343,7 +344,7 @@ class TaskInstance(Base, LoggingMixin):
task_id = Column(String(ID_LEN, **COLLATION_ARGS), primary_key=True, nullable=False)
dag_id = Column(String(ID_LEN, **COLLATION_ARGS), primary_key=True, nullable=False)
run_id = Column(String(ID_LEN, **COLLATION_ARGS), primary_key=True, nullable=False)
- map_index = Column(Integer, primary_key=True, nullable=False, default=-1)
+ map_index = Column(Integer, primary_key=True, nullable=False, server_default=text("-1"))
Review comment:
This one can possibly have default and server_default.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb edited a comment on pull request #20902: Switch map_index back to use server_default
Posted by GitBox <gi...@apache.org>.
ashb edited a comment on pull request #20902:
URL: https://github.com/apache/airflow/pull/20902#issuecomment-1014611361
Not _quite_ related to this, but when trying the migration from 2.2.3 to latest main I also get:
```
sqlalchemy.exc.ProgrammingError: (psycopg2.errors.UndefinedObject) constraint "task_reschedule_ti_fkey" of relation "task_reschedule" does not exist
```
(might be mixed version DB. let me drop and try) again
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr merged pull request #20902: Switch map_index back to use server_default
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #20902:
URL: https://github.com/apache/airflow/pull/20902
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb edited a comment on pull request #20902: Switch map_index back to use server_default
Posted by GitBox <gi...@apache.org>.
ashb edited a comment on pull request #20902:
URL: https://github.com/apache/airflow/pull/20902#issuecomment-1014611361
Not _quite_ related to this, but when trying the migration from 2.2.3 to latest main I also get:
```
sqlalchemy.exc.ProgrammingError: (psycopg2.errors.UndefinedObject) constraint "task_reschedule_ti_fkey" of relation "task_reschedule" does not exist
```
(might be mixed version DB. let me drop and try) again.
Confirmed on main: it doesn't upgrade on a fresh DB.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #20902: Switch map_index back to use server_default
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #20902:
URL: https://github.com/apache/airflow/pull/20902#issuecomment-1014611361
Not _quite_ related to this, but when trying the migration from 2.2.3 to latest main I also get:
```sqlalchemy.exc.ProgrammingError: (psycopg2.errors.UndefinedObject) constraint "task_reschedule_ti_fkey" of relation "task_reschedule" does not exist
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #20902: Switch map_index back to use server_default
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20902:
URL: https://github.com/apache/airflow/pull/20902#discussion_r786414827
##########
File path: airflow/models/taskinstance.py
##########
@@ -343,7 +344,7 @@ class TaskInstance(Base, LoggingMixin):
task_id = Column(String(ID_LEN, **COLLATION_ARGS), primary_key=True, nullable=False)
dag_id = Column(String(ID_LEN, **COLLATION_ARGS), primary_key=True, nullable=False)
run_id = Column(String(ID_LEN, **COLLATION_ARGS), primary_key=True, nullable=False)
- map_index = Column(Integer, primary_key=True, nullable=False, default=-1)
+ map_index = Column(Integer, primary_key=True, nullable=False, server_default=text("-1"))
Review comment:
I checked some sources and realised `default` isn’t actually much useful in ORM, only if we use the SQL expression builder. And with `server_default` set, `default` only really matters for `UPDATE`, which actually seems a bit wrong to me, so I’ll omit `default` until they are proven useful.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org