[GitHub] noronham commented on issue #3985: [AIRFLOW-3138] Use current data type for migrations

2018-10-02 Thread GitBox
noronham commented on issue #3985: [AIRFLOW-3138] Use current data type for 
migrations
URL: 
https://github.com/apache/incubator-airflow/pull/3985#issuecomment-426501635
 
 
   I did a little more testing today with mysql, and while the mysql version 
I'm using doesn't currently set 
   explicit_defaults_for_timestamp (so I can't upgrade to current master at the 
moment) - the migration for this runs fine both with and without my diff.
   
   On an unrelated note since I was testing a bit more using master instead of 
1.10.0 - there seems to be two current heads for migrations which makes it a 
little harder to run upgrades and downgrades (you have to explicitly upgrade to 
cc1e65623dc7)
   9635ae0956e7 -> 0a2a5b66e19d (head), add task_reschedule table
   9635ae0956e7 -> dd25f486b8ea (head)
   
   Let me know if there is anything else I should/could do before this can be 
merged!
   
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] noronham commented on issue #3985: [AIRFLOW-3138] Use current data type for migrations

2018-10-02 Thread GitBox
noronham commented on issue #3985: [AIRFLOW-3138] Use current data type for 
migrations
URL: 
https://github.com/apache/incubator-airflow/pull/3985#issuecomment-426311793
 
 
   @ashb - the problem I ran into was that (at least for postgres) - the data 
was being read from a naive timestamp column in the db (pre 
0e2a74e0fc9f_add_time_zone_awareness.py and ) and so the python code was 
converting it to an assumed UTC date. The session.merge of the task instance 
then fails because (at least in postgres) the timestamp with timezone is passed 
into the query as a full string and so the key no longer matches the database 
and you get an error and the migration fails:
   **'sqlalchemy.orm.exc.StaleDataError: UPDATE statement on table 
'task_instance' expected to update 1 row(s); 0 were matched.'**
   
   I don't think that the migration really needs the date - it just needs the 
primary key and so while for runtime code/etc - I agree 100% that the 
UtcDateTime should be used. However, I believe in this case since the database 
during this migration is storing naive dates AND we are only using the date as 
a value type - it should be safe  to keep naive in the migration code.
   
   I'll see if I can test with mysql and confirm this doesn't cause any new 
errors.
 * install airflow 1.8 or 1.9
 * run a task (so task_instance table isn't empty)
 * upgrade to 1.10
   
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services