Repository navigation
Conversation
91cfbe1 to
803b94e
Compare
A ResumableJobMixin retry increments resumable_job.reconnect_attempt and then one of reconnect_success / already_succeeded / terminal_resubmit, but only once the prior job's status has been fetched and classified. When that status check raises (external system unreachable, expired id, and so on) the attempt resolves to no outcome: the failure is silent and reconnect_attempt no longer reconciles with its outcome counters. Reconnection failure is the signal an operator most wants to alert on, yet today it cannot be measured. Signed-off-by: 1fanwang <1fannnw@gmail.com>
160601a to
819fb21
Compare
potiuk
added
the
ready for maintainer review
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Nice piece of work — the gap is real, and the E2E reconciliation table is better evidence than most metrics PRs bring. Four active, three succeeded, two terminal, five raising, and the attempt counter balancing only with the new metric is exactly the right way to demonstrate it.
One thing I'd like resolved, and it's aimed squarely at that invariant: the try is wider than it needs to be, so a logging call failing after an outcome counter has already fired would emit reconnect_failure as well, and reconnect_attempt would no longer reconcile. Details and a suggested narrowing inline on resumablejobmixin.py:189. The same widening makes the span's resumable.decision attribute overwrite a decision that was already recorded.
The test can't currently catch this: both parametrized cases raise before any outcome counter fires — see test_resumablejobmixin.py:299.
Nothing else from me. The YAML entry matches its siblings and the registry hook is green; no newsfragment needed, since none of the five existing resumable_job.* metrics has one; and the test follows the file's own patching convention, so I've no style notes. Note the branch is 876 commits behind main, so it'll want a rebase before it merges.
This review was drafted by an AI-assisted tool and confirmed by an Airflow maintainer. The findings below are observations, not blockers; an Airflow maintainer — a real person — will take the next look at the PR. If you think a finding is mis-applied, please reply on the PR and a maintainer will weigh in.
More on how Airflow handles maintainer review: contributing-docs/05_pull_requests.rst.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
| external_id=external_id, | ||
| status=status, | ||
| ) | ||
| except Exception: |
There was a problem hiding this comment.
The try starts at line 152 and wraps the branch bodies, not just the decision — so an outcome counter and this one can both fire for a single attempt.
Each branch increments and then logs: :161 → :162, :172 → :173, :182 → :183. If one of those logging calls raises, the outcome has already been counted and this except adds reconnect_failure on top, giving reconnect_attempt 1 and outcomes 2. That's the reconciliation your E2E table demonstrates, and it's the thing this PR exists to establish. Line 193 has the same problem in the trace: it overwrites a resumable.decision that was already set to reconnect / already_succeeded / terminal_resubmit.
Worth saying why I don't think this is academic: self.log is structlog, and remote log processors are injected into the global chain — #66633 is currently open about exactly that path. A logging call failing because a remote sink is unreachable is the same class of event this metric is meant to catch.
Narrowing the try to the fetch and classification, then branching on the result outside it, makes the exclusivity structural:
try:
status = self.get_job_status(external_id, context)
if self.is_job_active(status):
decision = "reconnect"
elif self.is_job_succeeded(status):
decision = "already_succeeded"
else:
decision = "terminal_resubmit"
except Exception:
span.set_attribute("resumable.decision", "reconnect_failure")
stats.incr("resumable_job.reconnect_failure", tags=stats_tags)
raise| assert "resumable_job.reconnect_success" not in called_names | ||
| assert "resumable_job.fresh_submit" not in called_names | ||
|
|
||
| @pytest.mark.parametrize("failing_method", ["get_job_status", "is_job_active"]) |
There was a problem hiding this comment.
Both parameters here raise before any outcome counter fires, so they're the two positions where double-counting can't happen — the test passes for the same reason the bug hides.
Two additions worth having: "is_job_succeeded" (the third classification call, at resumablejobmixin.py:169) is one more token in this list; and a case where the log call raises after an outcome was counted would pin the point in my comment on line 189 — it fails today, and passes once the try is narrowed.
Classify stored job status before recording the outcome. Keep logging errors from adding a second outcome or replacing the span decision. Signed-off-by: 1fanwang <1fannnw@gmail.com>
potiuk
added
the
closed because of open PR limit
|
Hello @1fanwang - thank you for your contributions to Apache Airflow! The Airflow community has introduced a limit of 5 open pull requests at a time for contributors without write access to the repository. You currently have 34 open pull requests, so - as a one-time step of introducing the limit - we closed the ones where maintainers have not engaged yet:
These pull requests stay open because maintainers are already engaged in them - they count towards your limit:
This is not a judgement of you or of your changes. We never told contributors before that opening many pull requests at once was a problem, so there is nothing to feel bad about - and nothing is lost: your branches, commits and the review history stay where they are. What we ask you to do is to make your first prioritization decision: choose which of the pull requests above matter most to you, and reopen them (up to 5 open at a time, including the ones still open) with the "Reopen pull request" button or While your pull requests are waiting for review, the most valuable thing you can do is help in other ways - reviewing other contributors' pull requests, helping with issues, and taking part in the discussions on the devlist and Slack. Why we introduced the limit, what it means for you and how to reopen or restore a pull request is explained in https://github.com/apache/airflow/blob/main/contributing-docs/32_open_pull_request_limit.rst. Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting |
When a task retry cannot fetch or classify its stored external job's status,
dashboards count a reconnect attempt without an outcome. This adds
resumable_job.reconnect_failureso operators can measure failed reconnects andreconcile attempts with outcomes.
The failure counter covers status lookup and classification. Outcome counters and
logging now run after classification, so a logging error cannot add a second
outcome or overwrite the trace decision. Exceptions still propagate.
Testing
The regression tests cover status lookup, both classifiers, and logging failures
for active, succeeded and failed jobs. To compare the original PR base with the
final head, keep the final tests and replace only the implementation. In a
disposable checkout with the SDK test dependencies installed:
Before, each status error emitted only an attempt; the failure counter assertion
failed. After, those errors emit one failure outcome. Logging errors preserve
their chosen outcome and trace decision.
The existing StatsD -> statsd-exporter -> Prometheus -> Grafana result recorded 14
attempts: four active, three succeeded, two terminal and five failures, plus six
fresh submissions. The outcomes sum to 14 instead of nine without the failure
counter. That stack was not rerun; the comparison above is local SDK evidence.
Was generative AI tooling used to co-author this PR?
Generated-by: GitHub Copilot CLI (GPT-6 Astra) following the guidelines