bingqin2 commented on PR #76843:
URL: https://github.com/apache/airflow/pull/76843#issuecomment-6832063522
Thanks for the review.
1. I left the reset out on purpose.
- The supervisor has no process of its own. It runs inside the executor's
worker process, a Celery pool process for example, and no Airflow code sets
`SIGCHLD` today. A reset to `SIG_DFL` there also changes how that process reaps
its other children: in a quick check, a child that the host never waits for
stays a zombie after the reset.
- It would not replace this change. A `waitpid(-1)` elsewhere in the
process loses the status the same way, and which path #70117 hits is not
confirmed.
I tried the reset at 05d4635a24 with a task process that kills itself
with `SIGKILL`. With `SIGCHLD` ignored, the exit code is `-1` and the task log
has the new warning. With the reset, it is `-9` and the log has the OOM
message. The final state is the same in both: `UP_FOR_RETRY` when retries
remain, `FAILED` otherwise. So retries already work, and the reset would bring
back the signal-specific log message. I can add it here or in a follow-up if a
maintainer wants it.
2. Yes, that is intended. `expect_signal` only silences the messages about
the exit code, and there is no exit code to report here. The warning is logged
once per process, and `kill()` then logs `Process exited` with `exit_code=-1`.
3. Added a comment above the `signal.signal()` call.
--
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]
