[GitHub] [airflow] ashb commented on a change in pull request #22184: Add run_id and map_index to SlaMiss

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

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

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