[
https://issues.apache.org/jira/browse/AIRFLOW-6569?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17020350#comment-17020350
]
ASF GitHub Bot commented on AIRFLOW-6569:
-----------------------------------------
mikeclarke commented on pull request #7232: [AIRFLOW-6569] Flush pending Sentry
exceptions before exiting forked process
URL: https://github.com/apache/airflow/pull/7232
After switching to `os.fork()` for the task runner, there is the possibility
that exceptions queued by Sentry will not be emitted prior to the process
exiting.
This fixes AIRFLOW-6569 by explicitly flushing pending exceptions prior to
calling `os._exit()` within the forked task runner.
This is covered by existing unit tests within
`tests/task/task_runner/test_standard_task_runner.py`.
---
Issue link: WILL BE INSERTED BY
[boring-cyborg](https://github.com/kaxil/boring-cyborg)
Make sure to mark the boxes below before creating PR: [x]
- [X] Description above provides context of the change
- [X] Commit message/PR title starts with `[AIRFLOW-NNNN]`. AIRFLOW-NNNN =
JIRA ID<sup>*</sup>
- [X] Unit tests coverage for changes (not needed for documentation changes)
- [X] Commits follow "[How to write a good git commit
message](http://chris.beams.io/posts/git-commit/)"
- [X] Relevant documentation is updated including usage instructions.
- [X] I will engage committers as explained in [Contribution Workflow
Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
<sup>*</sup> For document-only changes commit message can start with
`[AIRFLOW-XXXX]`.
---
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).
Read the [Pull Request
Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)
for more information.
----------------------------------------------------------------
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:
us...@infra.apache.org
> Broken sentry integration
> -------------------------
>
> Key: AIRFLOW-6569
> URL: https://issues.apache.org/jira/browse/AIRFLOW-6569
> Project: Apache Airflow
> Issue Type: Bug
> Components: configuration, hooks
> Affects Versions: 2.0.0, 1.10.7
> Reporter: Robin Edwards
> Priority: Minor
>
> I believe the new forking mechanism AIRFLOW-5931 has unintentionally broken
> the sentry integration.
> Sentry relies on the atexit
> http://man7.org/linux/man-pages/man3/atexit.3.html signal to flush collected
> errors to their servers. Previously as the task was executed in a new process
> as opposed to forked this got invoked. However now os._exit() is called
> (which is semantically correct with child processes)
> https://docs.python.org/3/library/os.html#os._exit
> Point os._exit is called in airflow:
> https://github.com/apache/airflow/pull/6627/files#diff-736081a3535ff0b9e60ada2f51154ca4R84
> Also related on sentry bug tracker:
> https://github.com/getsentry/sentry-python/issues/291
> Unfortunately sentry doesn't provide (from what i can find) a public
> interface for flushing errors to their system. The return value of their
> init() functions returns an object containg a client but the property is
> `_client` so it would be wrong to rely on it.
> I've side stepped this in two ways, you can disable the forking feature
> through patching CAN_FORK to False. But after seeing the performance
> improvement on my workers I opted to monkey patch the whole _exec_by_fork()
> and naughtily call sys.exit instead as a temporary fix.
> I personally dont find the actual sentry integration in airflow useful as it
> doesn't collect errors from the rest of the system only tasks. I've been
> wiring it in through my log config module since before the integration was
> added however its still effected by the above change.
> My personal vote (unless anyone has a better idea) would be to remove the
> integration completely document the way of setting it up through the logging
> class and providing a 'post_execute' hook of some form on the
> StandardTaskRunner where people can flush errors using what not.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
