[GitHub] [airflow] notatallshaw-gts commented on pull request #27111: Update SLA wording to reflect it is relative to Dag Run start
notatallshaw-gts commented on PR #27111: URL: https://github.com/apache/airflow/pull/27111#issuecomment-1313106307 > With my Airflow (1.14.15), the SLA miss was not detected in your scenario (#26566) until the second DAG run. > > I might have found the reason. Airflow 1.14.15 does check for the SLA one interval ahead of each task, but it does so for SUCCESS/SKIPPED tasks only, not the currently running tasks. When a new run is triggered, the SUCCESS/SKIPPED tasks from the _previous_ run will be one interval behind, so looking one interval ahead works for these tasks. That’s when the SLA miss is triggered. Definitely later than I expected. Do you mean 1.10.15? Regardless this fits with the experience of SLAs I had with Airflow 1.x era, that is they came much later than I expected and I didn't find them useful Fortunately I currently only have to work with Airflow 2.x instances, and I have been setting up a lot of SLAs recently and my practical experience of them is they work as I've described in this PR. -- 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
[GitHub] [airflow] notatallshaw-gts commented on pull request #27111: Update SLA wording to reflect it is relative to Dag Run start
notatallshaw-gts commented on PR #27111: URL: https://github.com/apache/airflow/pull/27111#issuecomment-1312176736 > I confirm that the first SLA check is counter-intuitively relative to a date in the future. I'm not sure I completely follow this working. Do you have a concrete example where this differs from the wording in my Pull request? The wording I have given matches the motivating example I give in #26566. > After all, it's fair to assume that all tasks should finish before the next DAG run. I'm not sure I follow what you're getting but no this is not a fair assumption. I have worked with many DAGs that run daily but each DAG run may be for longer than one (sometimes weeks). -- 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
[GitHub] [airflow] notatallshaw-gts commented on pull request #27111: Update SLA wording to reflect it is relative to Dag Run start
notatallshaw-gts commented on PR #27111: URL: https://github.com/apache/airflow/pull/27111#issuecomment-1311920464 > I am not advocating for re-opening this issue now, but I wonder if it might still lack clarity? > > Contrary to what one would intuitively expect, isn't the SLA relative to the _next_ DAG run? The code calls `next_dagrun_info()` twice before any SLA check. The first time to calculate the end of the current interval, and the second time to calculate the end of the next interval, which could be one day in the future if the DAG is scheduled to run once a day. > > https://github.com/apache/airflow/blob/cc4cde987cbc073a223b531a7674856cb2847f9a/airflow/dag_processing/processor.py#L421 That looks like it's calling next only once? `next_dagrun_info` is the outer call and `get_run_data_interval` is the inner call. And isn't the fact it calls `next_dagrun_info` an artifact of Airflow's quirky execution date behavior? Traditionally you needed to calculate the execution date of the of the next dag run to understand the "real" time that current dag starts at. I wonder if this could now be replaced with using `data_interval_end` from the current dag? Which if so might be less error prone? I'm happy for another PR to be opened for either even better clarity or updating this logic to be more consistent. -- 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
[GitHub] [airflow] notatallshaw-gts commented on pull request #27111: Update SLA wording to reflect it is relative to Dag Run start
notatallshaw-gts commented on PR #27111: URL: https://github.com/apache/airflow/pull/27111#issuecomment-1293526817 @potiuk Whenever you get a chance, any objections? -- 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
[GitHub] [airflow] notatallshaw-gts commented on pull request #27111: Update SLA wording to reflect it is relative to Dag Run start
notatallshaw-gts commented on PR #27111: URL: https://github.com/apache/airflow/pull/27111#issuecomment-1282856619 I don't think the build doc error is related to this change? At least as I'm interpreting the log output. -- 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
