ashb commented on a change in pull request #22184:
URL: https://github.com/apache/airflow/pull/22184#discussion_r828502055



##########
File path: airflow/dag_processing/processor.py
##########
@@ -412,20 +410,20 @@ def manage_slas(self, dag: DAG, session: Session = None) 
-> None:
             else:
                 while next_info.logical_date < ts:
                     next_info = dag.next_dagrun_info(next_info.data_interval, 
restricted=False)
-
                     if next_info is None:
                         break
-                    if (ti.dag_id, ti.task_id, next_info.logical_date) in 
recorded_slas_query:
+                    next_run_id = DR.generate_run_id(DagRunType.SCHEDULED, 
next_info.logical_date)
+                    if (ti.dag_id, ti.task_id, next_run_id, ti.map_index) in 
recorded_sla_misses:

Review comment:
       I think this behaviour is _even_ more confusing. Let's not touch SlaMiss 
_at all_, and instead create a parse-time-error if someone tries to set an 
`sla` property on a mapped task.
   
   (i.e. throw away most of this PR, sorry.)
   
   My reason: the current SLA behaviour of creating the SlaMiss record against 
the next execution date is confusing (and likely wrong) so lets not confuse 
matters more by changing it to by against a future run_id that _may never 
exist_. 
   
   We can come back and re-visit this once we have made SlaMiss more sensible.




-- 
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-unsubscr...@airflow.apache.org

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


Reply via email to