Repository navigation
Add warning for HttpOperator deferrable with non-idempotent methods - #69748
Conversation
This comment was marked as spam.
This comment was marked as spam.
potiuk
added
the
ready for maintainer review
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Thanks — the underlying point is a real one: a deferred non-idempotent request can be re-sent if the Triggerer restarts mid-flight, and IDEMPOTENT_METHODS is correctly populated per RFC 7231 (PUT and DELETE are idempotent even though they aren't safe).
My hesitation is about the delivery mechanism rather than the diagnosis. The warning is raised in HttpTrigger.__init__, then deliberately suppressed again in the operator to avoid a duplicate — a warn-then-unwarn round trip that's coupled to the message wording. I think there's a simpler shape here; details inline.
As with the diagnosis itself, these are design questions rather than defects — I'd rather you formed your own view than took mine as given. My review here was AI-assisted, and I'd guess parts of this PR were too; that's fine on both sides, but neither of us should treat the output as authoritative. Push back where you think I've got it wrong. I'd also welcome other maintainers' views on whether a UserWarning is the right channel for this at all.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
This comment was marked as spam.
This comment was marked as spam.
|
Did you manually review it and run it locally on your own and verified it works @Vamsi-klu ? Can you confirm it that it was not an agent doing static code and mechanical answer to it ? |
This comment was marked as spam.
This comment was marked as spam.
@Vamsi-klu Jarek asked if you reviewed it yourself, and not an agent, and you gave an agent-written response. That does not look encouraging. |
ashb
added
pending-response
and removed
ready for maintainer review
This comment was marked as spam.
This comment was marked as spam.
fdc5bee to
55277e6
Compare
HttpOperator(deferrable=True) executes the HTTP request inside HttpTrigger.run() in the Triggerer. On triggerer restart, the trigger is re-created from serialize() and run() re-executes, causing duplicate POST/PATCH requests. This is silent data duplication. Add UserWarning when deferrable=True is used with POST/PATCH etc, guiding users to idempotent methods or sensor-based polling pattern like AirbyteOperator (side-effect in worker, trigger polls with GET). Long-term fix should execute non-idempotent requests in worker and only poll in trigger. Fixes: apache#67945
The deferrable HTTP warning should stay visible for unsafe methods without showing the operator and trigger versions together, so the shared wording now has focused coverage for both entry points.
Trigger reconstruction can happen repeatedly in the Triggerer, where warnings do not reach Dag authors. Surface the guidance where the deferrable choice is made without producing repeated trigger-side noise.
…hods Address review feedback on the delivery mechanism: - Emit a single self.log.warning from HttpOperator.execute() when deferrable=True and the method is not idempotent (RFC 9110 §9.2.2), so Dag authors see the advisory in task logs once per attempt. - Do not warn from __init__ (parse-time flood) or HttpTrigger (Triggerer reconstructs from serialized kwargs on every resume/restart). - No catch_warnings/filterwarnings suppression path. - Cover method case, None, pagination cardinality, PUT/DELETE silence, PATCH/custom methods, deferrable=False, and trigger deserialize. Fixes: apache#67945
Dag authors who accept Triggerer-restart retry risk need a per-task opt-out, and task logs should point at provider docs rather than a GitHub issue that operators cannot act on.
Dag authors who intentionally POST in deferrable mode had no documented opt-out, and the advisory pointed at a GitHub issue instead of provider docs.
78d82a4 to
5a905f3
Compare
…pache#69748) * Add warning for HttpOperator deferrable with non-idempotent methods HttpOperator(deferrable=True) executes the HTTP request inside HttpTrigger.run() in the Triggerer. On triggerer restart, the trigger is re-created from serialize() and run() re-executes, causing duplicate POST/PATCH requests. This is silent data duplication. Add UserWarning when deferrable=True is used with POST/PATCH etc, guiding users to idempotent methods or sensor-based polling pattern like AirbyteOperator (side-effect in worker, trigger polls with GET). Long-term fix should execute non-idempotent requests in worker and only poll in trigger. Fixes: apache#67945 * Avoid duplicate HTTP deferrable warnings The deferrable HTTP warning should stay visible for unsafe methods without showing the operator and trigger versions together, so the shared wording now has focused coverage for both entry points. * Warn once for non-idempotent deferrable HTTP methods Trigger reconstruction can happen repeatedly in the Triggerer, where warnings do not reach Dag authors. Surface the guidance where the deferrable choice is made without producing repeated trigger-side noise. * Warn once from HttpOperator.execute for non-idempotent deferrable methods Address review feedback on the delivery mechanism: - Emit a single self.log.warning from HttpOperator.execute() when deferrable=True and the method is not idempotent (RFC 9110 §9.2.2), so Dag authors see the advisory in task logs once per attempt. - Do not warn from __init__ (parse-time flood) or HttpTrigger (Triggerer reconstructs from serialized kwargs on every resume/restart). - No catch_warnings/filterwarnings suppression path. - Cover method case, None, pagination cardinality, PUT/DELETE silence, PATCH/custom methods, deferrable=False, and trigger deserialize. Fixes: apache#67945 * Allow silencing HttpOperator deferrable non-idempotent warnings Dag authors who accept Triggerer-restart retry risk need a per-task opt-out, and task logs should point at provider docs rather than a GitHub issue that operators cannot act on. * Add a silence flag and docs for the HttpOperator deferrable warning Dag authors who intentionally POST in deferrable mode had no documented opt-out, and the advisory pointed at a GitHub issue instead of provider docs. --------- Co-authored-by: probe <probe@example.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com>
What is the change?
HttpOperator.execute()now logs one warning per task attempt whendeferrable=Trueand the HTTP method is outside the RFC 9110 idempotent set (GET,HEAD,OPTIONS,PUT,DELETE,TRACE). A newwarn_on_non_idempotentflag (defaultTrue) silences it, and the deferrable docs explain the risk.Why did I do it?
Fixes: #67945
A deferrable
HttpOperatorsends the request insideHttpTrigger.run()on the triggerer. If the triggerer restarts before completion, the trigger is rebuilt from serialized kwargs and can send the request again. That is dangerous for non-idempotent methods, including the defaultPOST, and nothing told Dag authors.How did I do it?
The warning lives in
_warn_if_deferrable_non_idempotent_method, called only fromexecute(), so it reaches the Dag author in task logs once per attempt. Construction does not warn, which avoids flooding the Dag processor at parse time, and the trigger never warns because triggerer logs are the wrong audience. Paginated runs still warn once. Docs indeferrable.rstandoperators.rstcover the risk and the silence flag.What's the impact?
No change to request execution. Dag authors get an explicit task-log advisory when deferring non-idempotent methods, and can opt out per task with
warn_on_non_idempotent=False.What's the test plan?
pytest providers/http/tests/unit/http -qpassed (120 passed, 1 skipped). New parametrized tests assert exact warning counts per method and deferrable combination, the silence flag, no warning at construction,method=Nonehandling, and a single warning for paginated runs.