kaxil commented on PR #67592:
URL: https://github.com/apache/airflow/pull/67592#issuecomment-5942860558
The code looks right to me now. A few non-blocking fixes before merge:
1. The PR description still describes the old guard. It says retries
(`end_date` set) and deferral resumes (`next_method`) are skipped, and it names
a `test_ti_run_skips_queued_duration_metric` that no longer exists. The
description becomes the squash commit message, so could you update it to say
the metric now emits once per queue wait (first run, retry, deferral resume)
and skips only when `queued_dttm` is unset or on a duplicate start?
2. In the comment in `ti_run`, "task.scheduled_duration counts per try
instead" isn't accurate. `emit_state_change_metric` returns early whenever
`end_date` is set, so `scheduled_duration` skips retries but does emit on
deferral resumes. Maybe: "task.scheduled_duration skips retries
(emit_state_change_metric returns early while end_date is set), so the two
disagree on retries by design." The next line and the `_without_queued_dttm`
test docstring still say "rare races". Per your reply to @samraj2k, the real
case is runs that skip the scheduler's queueing, e.g. `dag.test()`.
3. The newsfragment says deferral resumes now report their queue wait, but
Airflow 2 already did that. `_defer_task` never set `end_date`, so
`emit_state_change_metric` fired on resume. What actually changes from 2.x is
retries and reschedule-mode sensor pokes, both of which 2.x skipped because
they leave `end_date` set (`_handle_reschedule` sets it). A reschedule sensor
that pokes every minute for an hour goes from one sample to about 60. That will
move percentiles for anyone upgrading from 2.x who alerts on this timer. It
fits "once per queue wait", and I left the reschedule path out when I listed
the transitions earlier, but the newsfragment should say so.
4. Optional: the emit runs before code that can still fail, namely the
`invalid_arg_bindings` 500, `issue_execution_token`, and the commit. The SDK
client retries 5xx responses and the rollback puts the TI back in QUEUED, so
the retried request emits a second sample. Moving the `if
emit_queued_duration:` block to just before `return context` leaves only the
commit able to fail after the emit.
5. Nit: the new `mock.patch("...task_instances.stats")` calls could take
`autospec=True`, like other patches in this file do.
--
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]
