hkc-8010 commented on PR #66911: URL: https://github.com/apache/airflow/pull/66911#issuecomment-5385347763
@jason810496 Fair point on the extra round-trip, I've reworked it to drop the pre-check. The probe now only runs inside the `except` for the ambiguous transport errors, so a trigger that succeeds costs exactly one request, same as before this PR. `test_trigger` asserts the request list is a single POST so it can't drift back. No extra pressure on the API server or DB on the normal path. On relying on the FK constraint: I agree the server will only ever hold one DagRun for that run ID. The problem this fixes is on the client side, where after a lost response it can't tell whether the run it finds is the one its own POST just created or one that was already there. The FK guarantees uniqueness but not authorship, so the client still has to ask. One behaviour change worth calling out now that the pre-check is gone: if a run genuinely already existed and the POST fails with an ambiguous transport error, `trigger()` returns OK instead of `DAGRUN_ALREADY_EXISTS`. That needs both things to happen in the same call. It's written up under "Behaviour change" in the PR description. If you'd rather keep that distinction I can compare the found run's `logical_date`/`conf` against what was sent, still without adding anything to the happy path. I also removed a few tests that duplicated the existing `test_trigger_conflict` pair, and collapsed the per-exception variants into parametrized ones. -- 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]
