Skip to content

Conversation

@XD-DENG
Copy link
Member

@XD-DENG XD-DENG commented Feb 2, 2023

Now the KubernetesExecutor supports method get_task_log() (in main branch). However, the number of lines to read is hard-coded to be 100.

It can be imagined that in many cases people may need more logs. And it would be nice to make this number of log lines to read from worker pod configurable. So that users can adjust according to their certain environment.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
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.

@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:Scheduler including HA (high availability) scheduler labels Feb 2, 2023
@XD-DENG
Copy link
Member Author

XD-DENG commented Feb 3, 2023

Hi @dstandish , mind helping take a look at this one? Thanks a lot!

@XD-DENG XD-DENG force-pushed the k8s-executor-log-fetching-lines-configurable branch from 8a27961 to b4e1269 Compare February 5, 2023 15:57
@XD-DENG XD-DENG requested a review from potiuk February 5, 2023 16:05
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe running_pod_tail_lines?

because it seems pretty rare that you'd want to customize this, just to avoid another config param i'm tempted to suggest using a class constant that you could monkey patch in airflow local settings... but... i don't feel strongly about it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dstandish . Renamed as suggested.

I fully understand the concern about adding another config param, but I still find this necessary. 100 lines seems reasonable for most cases, but when user want to have more flexibility, e.g. debugging their jobs, more log lines will be needed (this happens to me often in real works).

Comparing with different methods, like the one you shared above (class constant and monkey patch in local settings), I still find adding it as a config param is better.

Please let me know if these clarify, or should you have any other concern. Thanks a lot!

@XD-DENG XD-DENG force-pushed the k8s-executor-log-fetching-lines-configurable branch from b4e1269 to d4cbeb8 Compare February 12, 2023 00:29
@github-actions
Copy link

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.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Mar 30, 2023
@github-actions github-actions bot closed this Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Scheduler including HA (high availability) scheduler provider:cncf-kubernetes Kubernetes (k8s) provider related issues stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants