kaxil commented on PR #69597:
URL: https://github.com/apache/airflow/pull/69597#issuecomment-5942494962
LGTM overall. A few notes, happy for these to land here or in a follow-up:
1. `[logging] task_logs_to_stdout` reads as a global switch in `config.yml`,
in `celery_executor.rst` ("applies to all executors that supervise task
subprocesses") and in the newsfragment, but core only consults it when a caller
leaves `subprocess_logs_to_stdout` out of `run_workload`. LocalExecutor and the
per-task entrypoint in `execute_workload.py` (Kubernetes, ECS, Batch, Lambda)
pass `True`, so setting it to `False` there does nothing. Today the only
readers are the Celery worker and the edge3 worker's fork path on 3.4+. Could
the description say that, and drop "Disabled by default to preserve existing
behaviour", since most executors already forward?
2. The parity sentence in `provider.yaml` is off in two places.
LocalExecutor only forwards from 3.2.0, and a Kubernetes pod runs one task, so
its stdout never interleaves. On a Celery worker up to `worker_concurrency`
tasks share stdout, and raw print/stderr lines are forwarded with only
`logger="task.stdout"` and no task id (`forward_to_log` in the supervisor), so
a log collector can't tell which task a line came from. I'd drop the parity
sentence and mention the interleaving in the docs section. Binding TI identity
on the forwarding logger can be a separate task-sdk change.
`get_provider_info.py` needs regenerating after the edit.
3. Nit: the two new tests in `test_base_executor.py` repeat the same
`ExecuteTask` literal. Folding the explicit-argument case into the parametrize
as an `explicit` column would drop the copy and also cover the other direction
(conf `False`, explicit `True`).
The branch also conflicts with main in `test_base_executor.py` now, so it
needs a 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
