[GitHub] [airflow] ephraimbuddy commented on pull request #16401: Add 'queued' state to DagRun

[GitBox <gi...@apache.org>]

ephraimbuddy commented on pull request #16401:
URL: https://github.com/apache/airflow/pull/16401#issuecomment-872068158


   > I think setting `queued_at` should be handled internally inside `DagRun()` -- 99% of cases (maybe everything outside of tests) will always want to use "now" for that, so we should do something like we do for variables:
   > 
   > ```python
   > class DagRun:
   > 
   >     __NO_VALUE = object()
   > 
   >     def __init__(
   >         self,
   >         state
   >         # ...
   >         queued_at = __NO_VALUE,
   >     ):
   >         self.state = state
   >         if queued_at is self.__NO_VALUE:
   >             self.queued_at = utcnow() if state == State.QUEUED else None
   >         else:
   >             self.queued_at = queued_at
   > ```
   > 
   > This special "no value" object lets us have a default value that is semantically distinct from None.
   > 
   > And by handling this internally in the DagRun class we then have less places in the code to update (and less places to get wrong). That said: we should also set `self.queued_at` in side `DagRun.set_state`
   > 
   > Wasn't there previously a migration in this PR that changed start_date on the DagRun table to be nullable? What happened to that?
   
   We had a migration but from @kaxil’s review we discovered we don’t need migration for `start_date` or the `state` because they both don’t have a `server_default` values set. Their values are python values and no server_default set for them. That was why I deleted the 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.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org