potiuk commented on code in PR #69748:
URL: https://github.com/apache/airflow/pull/69748#discussion_r3680737346
##########
providers/http/src/airflow/providers/http/triggers/http.py:
##########
@@ -141,6 +167,13 @@ def __init__(
self.headers = headers
self.data = data
self.extra_options = extra_options
+ warn_if_method_not_idempotent(
Review Comment:
Triggers are reconstructed from serialized kwargs every time the Triggerer
picks them up — `triggerer_job_runner.py:1386` does `trigger_instance =
trigger_class(**deserialised_kwargs)`. So this warning fires in the Triggerer
on every restart and resume, not just once at authoring time.
That's the wrong audience: by then the Dag is already written and deployed,
nobody is reading Triggerer warnings, and the person who could act on it (the
Dag author) never sees it. It's also the reason the operator has to suppress it
again.
Warning only from `HttpOperator` — where the author's `deferrable=True`
choice is actually being made — would reach the right person and remove the
need for any suppression.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/http/src/airflow/providers/http/operators/http.py:
##########
@@ -209,18 +216,36 @@ def paginate_sync(self, response: Response) -> Response |
list[Response]:
return all_responses
def execute_async(self, context: Context) -> None:
+ trigger = self._build_http_trigger(
+ endpoint=self.endpoint,
+ headers=self.headers,
+ data=self.data,
+ extra_options=self.extra_options,
+ )
self.defer(
- trigger=HttpTrigger(
+ trigger=trigger,
+ method_name="execute_complete",
+ )
+
+ def _build_http_trigger(self, **trigger_kwargs: Any) -> HttpTrigger:
+ warn_if_method_not_idempotent(
+ self.method,
+ subject="HttpOperator with deferrable=True",
+ execution_context="Deferrable mode executes the request in the
Triggerer",
+ alternative="HttpSensor/EventSensor",
+ method_connector="and",
+ stacklevel=3,
+ )
+ with warnings.catch_warnings():
Review Comment:
This suppresses the trigger's warning by matching
`NON_IDEMPOTENT_METHOD_WARNING_PATTERN = ".*may send duplicate requests.*"`
against the message text, which couples dedup to the exact wording. Reword the
message in `warn_if_method_not_idempotent` and this silently stops matching —
users get two warnings and no test necessarily catches it.
(To be clear on one thing I initially suspected and checked: there's no
`await` inside this block, so it isn't a concurrency hazard — it's the
indirection I'd push back on, not thread-safety.)
If the warning moves to the operator only, this whole block goes away.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/http/src/airflow/providers/http/triggers/http.py:
##########
@@ -60,6 +64,28 @@ def deserialize_auth_type(path: str | None) -> type | None:
return getattr(import_module(module_path), cls_name)
+def warn_if_method_not_idempotent(
Review Comment:
Five parameters — including `method_connector` existing purely to make
"HttpOperator with deferrable=True **and** method=POST" read grammatically
versus "HttpTrigger **with** method=POST" — is a lot of machinery for two call
sites that produce nearly the same sentence.
If the trigger-side call goes away per the comment above, this collapses to
something like `warn_if_method_not_idempotent(method)` with the message
inlined. `stacklevel=3` is also hardcoded to the current call depth and will
silently point at the wrong frame if the helper is ever called from elsewhere.
---
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]
