You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "BasPH (via GitHub)" <gi...@apache.org> on 2024/04/28 19:47:17 UTC
[PR] Set parallelism log messages to warning level for better visiblity [airflow]
BasPH opened a new pull request, #39298:
URL: https://github.com/apache/airflow/pull/39298
This PR raises the log level of several log messages related to parallelism to warning level. IMO it's currently not evident when Airflow reaches a parallelism limit so the purpose of this PR is to make it more obvious that a scalability limit is reached.
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
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/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.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
Re: [PR] Set parallelism log messages to warning level for better visiblity [airflow]
Posted by "RNHTTR (via GitHub)" <gi...@apache.org>.
RNHTTR commented on PR #39298:
URL: https://github.com/apache/airflow/pull/39298#issuecomment-2087625988
> > This is only true for users whose Airflow environment is somewhat optimized. There are a lot of configs to consider (parallelism, worker concurrency, pools) in order to limit task throughput -- surfacing more info to the user could only be helpful IMO.
>
> Exactly, in this case:
>
> 1. It would be useless to warn the user when the pool limit or dag/run/task max concurrency are reached, because these limits are usually set according to the capacity of an external system
> 2. Some users (including me) will find some of these warnings annoying especially that we can use the OTEL or statsd metrics to monitor the concurrency and create alerts.
Yeah that's fair. Do you agree that at least an info log is reasonable?
--
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
Re: [PR] Set parallelism log messages to warning level for better visiblity [airflow]
Posted by "RNHTTR (via GitHub)" <gi...@apache.org>.
RNHTTR commented on PR #39298:
URL: https://github.com/apache/airflow/pull/39298#issuecomment-2083783597
> I think this is more sutaible for cluster activity dashboard to expose this in actionable manner. Logs are a bit problematic because what you get here is notification that parallesim is reach but you have no information if user set this specific value by design or not. If user chose the specific value then logging warnings is not very user freidnly and in fact not actionable. This can result in further user request to silent the messages. In my point of view If we can find a way to present the information in cluster activity dashboard that would be prefered.
Why not both? I personally spend a lot more time debugging Airflow logs than I do using the cluster activity dashboard. If parallelism is reached unexpectedly such that it causes a problem, I'd definitely find it faster in the logs.
--
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
Re: [PR] Set parallelism log messages to warning level for better visiblity [airflow]
Posted by "RNHTTR (via GitHub)" <gi...@apache.org>.
RNHTTR commented on code in PR #39298:
URL: https://github.com/apache/airflow/pull/39298#discussion_r1585619655
##########
airflow/executors/base_executor.py:
##########
@@ -221,7 +221,10 @@ def heartbeat(self) -> None:
self.log.debug("%s running task instances", num_running_tasks)
self.log.debug("%s in queue", num_queued_tasks)
- self.log.debug("%s open slots", open_slots)
+ if open_slots == 0:
+ self.log.warning("Executor parallelism limit reached. 0 open slots.")
Review Comment:
```suggestion
self.log.info("Executor parallelism limit reached. 0 open slots.")
```
--
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
Re: [PR] Set parallelism log messages to warning level for better visiblity [airflow]
Posted by "BasPH (via GitHub)" <gi...@apache.org>.
BasPH commented on PR #39298:
URL: https://github.com/apache/airflow/pull/39298#issuecomment-2089948475
Okay I reverted a few logging statements so that now leaves an info log when `AIRFLOW__CORE__PARALLELISM` is reached, which should help users understand why Airflow isn't running any more tasks.
@hussein-awala Regarding:
> It would be useless to warn the user when the pool limit or dag/run/task max concurrency are reached, because these limits are usually set according to the capacity of an external system
Disagree with this statement. Airflow has lots of knobs to tweak for parallelism which can be a blessing and a curse. You might have a perfect setup with OTEL integration, dashboards, and alerts, but I'm sure that knowledge and tooling wasn't developed overnight. My experience is that Airflow's default settings quite often limit the user before they start reaching hardware limits. This is a frustrating problem because as of today visibility (via UI/logs) on such limits is low. I'll investigate alternative options such as dashboards, but stating that it's "useless to warn the user" is not my experience since 9 out of 10 times I see people hitting a default limit instead of a user-defined limit.
--
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
Re: [PR] Set parallelism log messages to warning level for better visiblity [airflow]
Posted by "BasPH (via GitHub)" <gi...@apache.org>.
BasPH commented on PR #39298:
URL: https://github.com/apache/airflow/pull/39298#issuecomment-2081635974
Why should we not warn the user?
--
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
Re: [PR] Set parallelism log messages to warning level for better visiblity [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on PR #39298:
URL: https://github.com/apache/airflow/pull/39298#issuecomment-2083828889
My 2c, I think info is appropriate.
--
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
Re: [PR] Set parallelism log messages to warning level for better visiblity [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #39298:
URL: https://github.com/apache/airflow/pull/39298#issuecomment-2086872578
> This is only true for users whose Airflow environment is somewhat optimized. There are a lot of configs to consider (parallelism, worker concurrency, pools) in order to limit task throughput -- surfacing more info to the user could only be helpful IMO.
Exactly, in this case:
1. It would be useless to warn the user when the pool limit or dag/run/task max concurrency are reached, because these limits are usually set according to the capacity of an external system
2. Some users (including me) will find some of these warnings annoying especially that we can use the OTEL or statsd metrics to monitor the concurrency and create alerts.
--
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
Re: [PR] Set parallelism log messages to warning level for better visiblity [airflow]
Posted by "BasPH (via GitHub)" <gi...@apache.org>.
BasPH commented on PR #39298:
URL: https://github.com/apache/airflow/pull/39298#issuecomment-2081639790
To inform the user that a software limit has been reached. For example, unless you enable debug logging there's no way to tell from the logs that `AIRFLOW__CORE__PARALLELISM` was reached. The default limits are rather low in Airflow so this is a common situation and it's currently not self-evident.
--
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
Re: [PR] Set parallelism log messages to warning level for better visiblity [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #39298:
URL: https://github.com/apache/airflow/pull/39298#issuecomment-2081638551
I think the question is why should we warn the user when the parallel limits are reached?
--
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
Re: [PR] Set parallelism log messages to warning level for better visiblity [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #39298:
URL: https://github.com/apache/airflow/pull/39298#issuecomment-2081664789
> To inform the user that a software limit has been reached. For example, unless you enable debug logging there's no way to tell from the logs that `AIRFLOW__CORE__PARALLELISM` was reached. The default limits are rather low in Airflow so this is a common situation and it's currently not self-evident.
I think this is more sutaible for cluster activity dashboard to expose this in **actionable** manner. Logs are a bit problematic because what you get here is notification that parallesim is reach but you have no information if user set this specific value by design or not. If user chose the specific value then logging warnings is not very user freidnly.
--
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