potiuk commented on code in PR #72376:
URL: https://github.com/apache/airflow/pull/72376#discussion_r3963311524
##########
providers/http/src/airflow/providers/http/triggers/http.py:
##########
@@ -298,6 +298,8 @@ class HttpEventTrigger(HttpTrigger, BaseEventTrigger):
:param data: Payload to be uploaded or request parameters.
:param extra_options: Additional kwargs to pass when creating a request.
:parama poll_interval: How often, in seconds, the trigger should send a
request to the API.
Review Comment:
Pre-existing typo — `:parama` should be `:param`, otherwise Sphinx renders
it as literal text rather than a parameter entry. Free to fix while you are in
this docstring adding the line below it.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/http/tests/unit/http/triggers/test_http.py:
##########
@@ -259,22 +261,97 @@ async def
test_trigger_on_success_yield_successfully(self, mock_hook, event_trig
assert event_trigger._run_response_check.call_count == 2
@pytest.mark.asyncio
+ @pytest.mark.parametrize("failing_call", ["request", "response_check"])
+ @mock.patch(HTTP_PATH.format("asyncio.sleep"), autospec=True)
@mock.patch(HTTP_PATH.format("HttpAsyncHook"))
- async def test_trigger_on_exception_logs_error_and_never_yields(
- self, mock_hook, event_trigger, monkeypatch
+ async def test_trigger_keeps_polling_after_an_exception(
+ self, mock_hook, mock_sleep, failing_call, event_trigger,
client_response, monkeypatch
):
"""
- Tests the HttpEventTrigger logs the appropriate message and does not
yield a TriggerEvent when an exception is raised.
+ Tests the HttpEventTrigger reports the failure and fires on a later
poll.
"""
- mock_hook.return_value.run.side_effect = Exception("Test exception")
+ error = HttpErrorException("503:Service Unavailable")
+ if failing_call == "request":
+ mock_hook.return_value.run.side_effect = [error,
self._mock_run_result(client_response)]
+ event_trigger._run_response_check =
mock.AsyncMock(return_value=True)
+ else:
+ mock_hook.return_value.run.return_value =
self._mock_run_result(client_response)
+ event_trigger._run_response_check =
mock.AsyncMock(side_effect=[error, True])
mock_logger = mock.Mock()
monkeypatch.setattr(type(event_trigger), "log", mock_logger)
+ response = await HttpEventTrigger._convert_response(client_response)
generator = event_trigger.run()
- with pytest.raises(StopAsyncIteration):
- await generator.asend(None)
+ actual = await generator.asend(None)
- mock_logger.error.assert_called_once_with("status: error, message:
%s", "Test exception")
+ assert actual == TriggerEvent(
+ {
+ "status": "success",
+ "response": HttpResponseSerializer.serialize(response),
+ }
+ )
+ assert mock_hook.return_value.run.call_count == 2
+ mock_sleep.assert_awaited_once_with(TEST_POLL_INTERVAL)
+ assert mock_logger.warning.call_count == 1
+ assert mock_logger.exception.call_count == 0
Review Comment:
`log.exception` is never called anywhere in `run()`, so this asserts the
absence of something that could not happen — it will pass no matter how the
code changes. Same at line 322. I would drop both; `assert
mock_logger.warning.call_count == 1` above already pins the logging behaviour
that matters.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/http/src/airflow/providers/http/triggers/http.py:
##########
@@ -311,10 +313,14 @@ def __init__(
data: dict[str, Any] | str | None = None,
extra_options: dict[str, Any] | None = None,
poll_interval: float = 60.0,
+ max_consecutive_failures: int = 10,
):
super().__init__(http_conn_id, auth_type, method, endpoint, headers,
data, extra_options)
self.response_check_path = response_check_path
self.poll_interval = poll_interval
+ if max_consecutive_failures < 1:
+ raise ValueError("max_consecutive_failures must be greater or
equal to 1")
Review Comment:
Non-blocking, and I would keep this validation as it is — but worth knowing
that it runs a second time in a place with much less forgiving error handling.
`create_triggers()` wraps `trigger_class(**deserialised_kwargs)` in `except
TypeError`. A `ValueError` escaping there propagates out of `create_triggers()`
into the runner's main-loop `except Exception`, which logs "Trigger runner
failed", sets `self.stop = True` and re-raises — taking every other trigger on
that triggerer with it.
Unreachable in practice: the DAG file constructs `HttpEventTrigger(...)` at
parse time, so an invalid value never reaches the DB. Flagging it because it
applies to any trigger that validates in `__init__`, and it seemed worth
surfacing given how carefully the rest of this PR reasons about failure modes.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
