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 2022/01/30 21:45:03 UTC
[GitHub] [airflow] avkirilishin opened a new pull request #21230: Fetch log file from multiple worker
avkirilishin opened a new pull request #21230:
URL: https://github.com/apache/airflow/pull/21230
closes: #16472
A robust solution requires TI try history. I'm not sure it's worth it. So this PR is a workaround: we try to find an appropriate host in all task instances. Some hosts can disappear from TI-table, but I think it's a rare case.
Solution tested at the 3-host cluster.
<!--
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 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 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/main/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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #21230: Fetch log file from multiple worker
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #21230:
URL: https://github.com/apache/airflow/pull/21230#discussion_r795404605
##########
File path: airflow/utils/log/file_task_handler.py
##########
@@ -159,26 +158,42 @@ def _read(self, ti, try_number, metadata=None):
except Exception as f:
log += f'*** Unable to fetch logs from worker pod {ti.hostname} ***\n{str(f)}\n\n'
else:
+ timeout = None # No timeout
+ try:
+ timeout = conf.getint('webserver', 'log_fetch_timeout_sec')
+ except (AirflowConfigException, ValueError):
+ pass
Review comment:
```suggestion
try:
timeout: Optional[int] = conf.getint('webserver', 'log_fetch_timeout_sec')
except (AirflowConfigException, ValueError):
timeout = None # No timeout
```
##########
File path: airflow/utils/log/file_task_handler.py
##########
@@ -19,18 +19,17 @@
import logging
import os
from pathlib import Path
-from typing import TYPE_CHECKING, Optional
+from typing import Optional
import httpx
from itsdangerous import TimedJSONWebSignatureSerializer
from airflow.configuration import AirflowConfigException, conf
+from airflow.models import TaskInstance
from airflow.utils.context import Context
from airflow.utils.helpers import parse_template_string, render_template_to_string
from airflow.utils.log.non_caching_file_handler import NonCachingFileHandler
-
-if TYPE_CHECKING:
- from airflow.models import TaskInstance
Review comment:
Better to keep this conditional to avoid potential circuliar import. `airflow.models` imports _way_ too many things it’s not a good idea to import it in `utils`.
--
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] potiuk commented on pull request #21230: Fetch log file from multiple worker
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21230:
URL: https://github.com/apache/airflow/pull/21230#issuecomment-1040813073
You need to rebase
--
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] avkirilishin commented on pull request #21230: Fetch log file from multiple worker
Posted by GitBox <gi...@apache.org>.
avkirilishin commented on pull request #21230:
URL: https://github.com/apache/airflow/pull/21230#issuecomment-1049157278
> You need to rebase
@potiuk Done
--
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