You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "john-jac (via GitHub)" <gi...@apache.org> on 2023/02/13 17:57:22 UTC
[GitHub] [airflow] john-jac opened a new pull request, #29511: Add Landing Times entry to UI docs
john-jac opened a new pull request, #29511:
URL: https://github.com/apache/airflow/pull/29511
The doc entry explains how Landing Times are calculated and how they relate to task adoption time. It includes a screenshot of the Landing Times tab.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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
[GitHub] [airflow] john-jac commented on a diff in pull request #29511: Add Landing Times entry to UI docs
Posted by "john-jac (via GitHub)" <gi...@apache.org>.
john-jac commented on code in PR #29511:
URL: https://github.com/apache/airflow/pull/29511#discussion_r1104894469
##########
docs/apache-airflow/ui.rst:
##########
@@ -160,6 +160,22 @@ DAG over many runs.
------------
+Landing Times
+.............
+Airflow landing times are calculated from the task's scheduled time to
+the time the task finishes, either with success or another state (see
+:ref:`concepts:task-instances`). When subtracting the task duration
+(above) from the landing time you will find the adoption time. If the
+adoption time exceeds the
+:ref:`task adoption timeout <config:celery__task_adoption_timeout>`
+then the task will be terminated and, if applicable, rescheduled.
Review Comment:
Great question @eladkal . We know this is an important factor for the Celery executor, but I do not think there is an equivalent for other executors. As such, should I pull this out as a note instead, or perhaps move that part of the comment out of the UI section and into the Celery config section that it references?
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #29511: Add Landing Times entry to UI docs
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #29511:
URL: https://github.com/apache/airflow/pull/29511#discussion_r1106354404
##########
docs/apache-airflow/ui.rst:
##########
@@ -160,6 +160,22 @@ DAG over many runs.
------------
+Landing Times
+.............
+Airflow landing times are calculated from the task's scheduled time to
+the time the task finishes, either with success or another state (see
+:ref:`concepts:task-instances`). When subtracting the task duration
+(above) from the landing time you will find the adoption time. If the
+adoption time exceeds the
+:ref:`task adoption timeout <config:celery__task_adoption_timeout>`
+then the task will be terminated and, if applicable, rescheduled.
Review Comment:
The UI part of the docs comes before the executors part. I'm not sure if it's wise to talk about executor specific before users are aware of what it is. Our docs design to be read as a flow from start to end (and many users on-board to Airflow in that manner)
WDYT?
--
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
[GitHub] [airflow] Taragolis commented on pull request #29511: Add Landing Times entry to UI docs
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #29511:
URL: https://github.com/apache/airflow/pull/29511#issuecomment-1442438064
> Sorry to have missed the checks. Should be good now @Taragolis
That was just friendly reminder, sometimes contributors forget about this parts (static checks) and how to solve it.
--
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
[GitHub] [airflow] john-jac commented on pull request #29511: Add Landing Times entry to UI docs
Posted by "john-jac (via GitHub)" <gi...@apache.org>.
john-jac commented on PR #29511:
URL: https://github.com/apache/airflow/pull/29511#issuecomment-1442365714
> @john-jac could you fix Static Checks, the only way in your case it is use `pre-commit`, see [Static code checks](https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#id4):
>
> * Changes from `airflow/config_templates/config.yml` should also applied to `airflow/config_templates/default_airflow.cfg` (pre-commit hooks will fix it)
>
> * You have redundant whitespaces in the end of the lines (pre-commit hooks will fix it too)
Sorry to have missed the checks. Should be good now @Taragolis
--
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
[GitHub] [airflow] Taragolis commented on pull request #29511: Add Landing Times entry to UI docs
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #29511:
URL: https://github.com/apache/airflow/pull/29511#issuecomment-1436048654
@john-jac could you fix Static Checks, the only way in your case it is use `pre-commit`, see [Static code checks](https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#id4):
- Changes from `airflow/config_templates/config.yml` should also applied to `airflow/config_templates/default_airflow.cfg` (pre-commit hooks will fix it)
- You have redundant whitespaces in the end of the lines (pre-commit hooks will fix it too)
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #29511: Add Landing Times entry to UI docs
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #29511:
URL: https://github.com/apache/airflow/pull/29511#discussion_r1104845329
##########
docs/apache-airflow/ui.rst:
##########
@@ -160,6 +160,22 @@ DAG over many runs.
------------
+Landing Times
+.............
+Airflow landing times are calculated from the task's scheduled time to
+the time the task finishes, either with success or another state (see
+:ref:`concepts:task-instances`). When subtracting the task duration
+(above) from the landing time you will find the adoption time. If the
+adoption time exceeds the
+:ref:`task adoption timeout <config:celery__task_adoption_timeout>`
+then the task will be terminated and, if applicable, rescheduled.
Review Comment:
BTW if this is executor specific calculation I wonder if it may be a better fit as tool tip in the dashboard where we can also raise the relevant tool tip for the relevant executor? (Not sure if this playes nicely with AIP-51 cc @o-nikolas ) but this feels more like one of the cases where information should be provided on the UI when user is actually viewing the metric?
--
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
[GitHub] [airflow] john-jac commented on a diff in pull request #29511: Add Landing Times entry to UI docs
Posted by "john-jac (via GitHub)" <gi...@apache.org>.
john-jac commented on code in PR #29511:
URL: https://github.com/apache/airflow/pull/29511#discussion_r1107387375
##########
docs/apache-airflow/ui.rst:
##########
@@ -160,6 +160,22 @@ DAG over many runs.
------------
+Landing Times
+.............
+Airflow landing times are calculated from the task's scheduled time to
+the time the task finishes, either with success or another state (see
+:ref:`concepts:task-instances`). When subtracting the task duration
+(above) from the landing time you will find the adoption time. If the
+adoption time exceeds the
+:ref:`task adoption timeout <config:celery__task_adoption_timeout>`
+then the task will be terminated and, if applicable, rescheduled.
Review Comment:
For now I've moved the celery-specific information to the config page entry and linked that back to the UI entry.
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #29511: Add Landing Times entry to UI docs
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #29511:
URL: https://github.com/apache/airflow/pull/29511#discussion_r1104842710
##########
docs/apache-airflow/ui.rst:
##########
@@ -160,6 +160,22 @@ DAG over many runs.
------------
+Landing Times
+.............
+Airflow landing times are calculated from the task's scheduled time to
+the time the task finishes, either with success or another state (see
+:ref:`concepts:task-instances`). When subtracting the task duration
+(above) from the landing time you will find the adoption time. If the
+adoption time exceeds the
+:ref:`task adoption timeout <config:celery__task_adoption_timeout>`
+then the task will be terminated and, if applicable, rescheduled.
Review Comment:
what is the explnation for other executors?
--
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
[GitHub] [airflow] eladkal merged pull request #29511: Add Landing Times entry to UI docs
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal merged PR #29511:
URL: https://github.com/apache/airflow/pull/29511
--
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
[GitHub] [airflow] o-nikolas commented on a diff in pull request #29511: Add Landing Times entry to UI docs
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #29511:
URL: https://github.com/apache/airflow/pull/29511#discussion_r1106346461
##########
docs/apache-airflow/ui.rst:
##########
@@ -160,6 +160,22 @@ DAG over many runs.
------------
+Landing Times
+.............
+Airflow landing times are calculated from the task's scheduled time to
+the time the task finishes, either with success or another state (see
+:ref:`concepts:task-instances`). When subtracting the task duration
+(above) from the landing time you will find the adoption time. If the
+adoption time exceeds the
+:ref:`task adoption timeout <config:celery__task_adoption_timeout>`
+then the task will be terminated and, if applicable, rescheduled.
Review Comment:
Yeah, this area is a real hodgepodge. I think landing times could be and are relevant to other executors (there is surely some delay between scheduling and the task actually running). But it's pronounced in Celery because of the queuing nature. But Celery isn't the only queuing type executor we'll likely have in the future or that people will build as a third party (think AWS Batch). So perhaps it makes more sense to move the logic for those metrics from `celery_executor.py` to `base_executor.py` at some point in time, and then the docs as well.
But for the moment, my 2c is that this doc contribution is useful to at least get down on paper somewhere and we can optimize the location in the future. I wouldn't block this PR for that.
--
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