You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/10/10 09:02:07 UTC
[GitHub] [airflow] turbaszek opened a new pull request #11392: Change some warning logs to info/debug
turbaszek opened a new pull request #11392:
URL: https://github.com/apache/airflow/pull/11392
It seems that some warning logs are confusing for our
users. This PR proposes to either change them to
info / debug level or improve the message.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #11392: Change some warning logs to info/debug
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11392:
URL: https://github.com/apache/airflow/pull/11392#discussion_r502777167
##########
File path: airflow/utils/dag_processing.py
##########
@@ -460,7 +460,7 @@ def _heartbeat_manager(self):
if self._process and not self._process.is_alive():
self._process.join(timeout=0)
if not self.done:
- self.log.warning(
+ self.log.debug(
"DagFileProcessorManager (PID=%d) exited with exit code %d - re-launching",
self._process.pid, self._process.exitcode
Review comment:
Won't there be more meaningful logs before this one?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #11392: Change some warning logs to info/debug
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11392:
URL: https://github.com/apache/airflow/pull/11392#issuecomment-791813250
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11392: Change some warning logs to info/debug
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11392:
URL: https://github.com/apache/airflow/pull/11392#discussion_r502770859
##########
File path: airflow/configuration.py
##########
@@ -342,10 +342,6 @@ def _get_option_from_default_config(self, section, key, **kwargs):
self.airflow_defaults.get(section, key, **kwargs))
else:
- log.warning(
Review comment:
I think this one should be kept - if we provide a `fallback` when getting a config this won't show up (I think), so seeing this warning is a clue to the user that they need to configure something
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #11392: Change some warning logs to info/debug
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11392:
URL: https://github.com/apache/airflow/pull/11392#discussion_r502958534
##########
File path: airflow/utils/dag_processing.py
##########
@@ -460,7 +460,7 @@ def _heartbeat_manager(self):
if self._process and not self._process.is_alive():
self._process.join(timeout=0)
if not self.done:
- self.log.warning(
+ self.log.debug(
Review comment:
```suggestion
warning(
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #11392: Change some warning logs to info/debug
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11392:
URL: https://github.com/apache/airflow/pull/11392#discussion_r502777429
##########
File path: airflow/configuration.py
##########
@@ -342,10 +342,6 @@ def _get_option_from_default_config(self, section, key, **kwargs):
self.airflow_defaults.get(section, key, **kwargs))
else:
- log.warning(
Review comment:
I thought that raising `AirflowConfigException` below will be enough
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11392: Change some warning logs to info/debug
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11392:
URL: https://github.com/apache/airflow/pull/11392#discussion_r502952470
##########
File path: airflow/configuration.py
##########
@@ -342,10 +342,6 @@ def _get_option_from_default_config(self, section, key, **kwargs):
self.airflow_defaults.get(section, key, **kwargs))
else:
- log.warning(
Review comment:
Oh I forgot about that :+1:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11392: Change some warning logs to info/debug
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11392:
URL: https://github.com/apache/airflow/pull/11392#discussion_r502770513
##########
File path: airflow/models/skipmixin.py
##########
@@ -61,7 +61,7 @@ def _set_state_to_skipped(self, dag_run, execution_date, tasks, session):
if execution_date is None:
raise ValueError("Execution date is None and no dag run")
- self.log.warning("No DAG RUN present this should not happen")
+ self.log.debug("No DAG run present. This should not happen.")
Review comment:
But it's a sign that _something_went wrong - having this at debug feels pointless
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #11392: Change some warning logs to info/debug
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11392:
URL: https://github.com/apache/airflow/pull/11392#discussion_r502767263
##########
File path: airflow/utils/dag_processing.py
##########
@@ -460,7 +460,7 @@ def _heartbeat_manager(self):
if self._process and not self._process.is_alive():
self._process.join(timeout=0)
if not self.done:
- self.log.warning(
+ self.log.debug(
"DagFileProcessorManager (PID=%d) exited with exit code %d - re-launching",
self._process.pid, self._process.exitcode
Review comment:
I think this should be `debug` because as I user I cannot do anything about it
##########
File path: airflow/utils/dag_processing.py
##########
@@ -518,7 +518,7 @@ def end(self):
:return:
"""
if not self._process:
- self.log.warning('Ending without manager process.')
Review comment:
I think this should be `debug` because as I user I cannot do anything about it
##########
File path: airflow/utils/dag_processing.py
##########
@@ -709,7 +709,7 @@ def _run_parsing_loop(self):
# This shouldn't happen, as in sync mode poll should block for
# ever. Lets be defensive about that.
- self.log.warning(
Review comment:
I think this should be `debug` because as I user I cannot do anything about 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11392: Change some warning logs to info/debug
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11392:
URL: https://github.com/apache/airflow/pull/11392#discussion_r502952612
##########
File path: airflow/utils/dag_processing.py
##########
@@ -460,7 +460,7 @@ def _heartbeat_manager(self):
if self._process and not self._process.is_alive():
self._process.join(timeout=0)
if not self.done:
- self.log.warning(
+ self.log.debug(
"DagFileProcessorManager (PID=%d) exited with exit code %d - re-launching",
self._process.pid, self._process.exitcode
Review comment:
No possibly not,at least not in the Scheduler log files - possibly no where if it dies with OOM or is killed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11392: Change some warning logs to info/debug
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11392:
URL: https://github.com/apache/airflow/pull/11392#discussion_r502770635
##########
File path: airflow/utils/dag_processing.py
##########
@@ -460,7 +460,7 @@ def _heartbeat_manager(self):
if self._process and not self._process.is_alive():
self._process.join(timeout=0)
if not self.done:
- self.log.warning(
+ self.log.debug(
"DagFileProcessorManager (PID=%d) exited with exit code %d - re-launching",
self._process.pid, self._process.exitcode
Review comment:
It's a sign that things are going wrong though, and it is definitely an exceptional conditions. I would say this should still be a warning
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #11392: Change some warning logs to info/debug
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11392:
URL: https://github.com/apache/airflow/pull/11392#discussion_r502777071
##########
File path: airflow/models/skipmixin.py
##########
@@ -61,7 +61,7 @@ def _set_state_to_skipped(self, dag_run, execution_date, tasks, session):
if execution_date is None:
raise ValueError("Execution date is None and no dag run")
- self.log.warning("No DAG RUN present this should not happen")
+ self.log.debug("No DAG run present. This should not happen.")
Review comment:
I agree, we should provide more useful information "This should not happen"
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #11392: Change some warning logs to info/debug
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11392:
URL: https://github.com/apache/airflow/pull/11392#issuecomment-751294283
Hey @turbaszek - should we rebase/reapply and merge this one?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #11392: Change some warning logs to info/debug
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11392:
URL: https://github.com/apache/airflow/pull/11392#discussion_r502767213
##########
File path: airflow/models/skipmixin.py
##########
@@ -61,7 +61,7 @@ def _set_state_to_skipped(self, dag_run, execution_date, tasks, session):
if execution_date is None:
raise ValueError("Execution date is None and no dag run")
- self.log.warning("No DAG RUN present this should not happen")
+ self.log.debug("No DAG run present. This should not happen.")
Review comment:
As a user I would have no idea what this log may mean
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #11392: Change some warning logs to info/debug
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #11392:
URL: https://github.com/apache/airflow/pull/11392#issuecomment-721446146
Can you please rebase your PR on latest Master since we have applied [Black](https://github.com/apache/airflow/commit/4e8f9cc8d02b29c325b8a5a76b4837671bdf5f68) and [PyUpgrade](https://github.com/apache/airflow/commit/8c42cf1b00c90f0d7f11b8a3a455381de8e003c5) on Master.
It will help if your squash your commits into single commit first so that there are less conflicts.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #11392: Change some warning logs to info/debug
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11392:
URL: https://github.com/apache/airflow/pull/11392#discussion_r502958534
##########
File path: airflow/utils/dag_processing.py
##########
@@ -460,7 +460,7 @@ def _heartbeat_manager(self):
if self._process and not self._process.is_alive():
self._process.join(timeout=0)
if not self.done:
- self.log.warning(
+ self.log.debug(
Review comment:
```suggestion
self.log.warning(
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] stale[bot] commented on pull request #11392: Change some warning logs to info/debug
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #11392:
URL: https://github.com/apache/airflow/pull/11392#issuecomment-751239954
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #11392: Change some warning logs to info/debug
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11392:
URL: https://github.com/apache/airflow/pull/11392#discussion_r517478395
##########
File path: airflow/jobs/scheduler_job.py
##########
@@ -260,7 +260,7 @@ def _kill_process(self) -> None:
raise AirflowException("Tried to kill process before starting!")
if self._process.is_alive() and self._process.pid:
- self.log.warning("Killing DAGFileProcessorProcess (PID=%d)", self._process.pid)
+ self.log.debug("Killing DAGFileProcessorProcess (PID=%d)", self._process.pid)
Review comment:
```suggestion
self.log.warning("Killing DAGFileProcessorProcess (PID=%d)", self._process.pid)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] closed pull request #11392: Change some warning logs to info/debug
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #11392:
URL: https://github.com/apache/airflow/pull/11392
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org