SameerMesiah97 commented on code in PR #77066:
URL: https://github.com/apache/airflow/pull/77066#discussion_r4569285923
##########
airflow-core/tests/unit/jobs/test_triggerer_job.py:
##########
@@ -1798,6 +1798,31 @@ def
test_run_trigger_on_kill_exception_does_not_swallow_cancelled_error(self, se
mock_trigger.on_kill.assert_awaited_once()
mock_trigger.cleanup.assert_awaited_once()
+ def test_run_trigger_cleanup_exception_is_logged(self, session,
cap_structlog) -> None:
+ """A cleanup() exception is logged as a warning and does not
propagate."""
+ trigger_runner = TriggerRunner()
+ trigger_runner.triggers = {
+ 1: {"task": MagicMock(spec=asyncio.Task), "is_watcher": False,
"name": "mock_name", "events": 0}
+ }
+ mock_trigger = MagicMock(spec=BaseTrigger)
+ mock_trigger.task_instance = MagicMock()
+ mock_trigger.task_instance.map_index = -1
+
+ async def run():
+ yield TriggerEvent("done")
+
+ mock_trigger.run = run
+ mock_trigger.cleanup = AsyncMock(side_effect=RuntimeError("cleanup
failed"))
+
+ asyncio.run(trigger_runner.run_trigger(1, mock_trigger))
+
+ mock_trigger.cleanup.assert_awaited_once()
+ assert {
+ "event": "cleanup() raised an exception",
+ "log_level": "warning",
+ "name": "mock_name",
+ } in cap_structlog
Review Comment:
Do we already have a test covering suppression of a cleanup exception? If
so, could we extend it with these logging assertions? Otherwise, this dedicated
test makes sense. Could we also assert that exception details are captured, so
removing `exc_info=True` would fail the test?
##########
airflow-core/newsfragments/77049.bugfix.rst:
##########
@@ -0,0 +1 @@
+Log a warning in the triggerer when a trigger's ``cleanup()`` method raises an
exception, instead of silently ignoring it, so failed cleanups (for example
unclosed clients or leaked resources) are visible in the triggerer log.
Review Comment:
newsfragments follow the naming convention [PR Number].bugfix.rst. This
should be named 77066.bugfix.rst. Arguably, this is not fixing a bug but
improving logging so perhaps 77066.improvement.rst might be better.
--
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]
