shahar1 commented on PR #66657:
URL: https://github.com/apache/airflow/pull/66657#issuecomment-4416119282
> LGTM — the diagnosis is right and the fix is minimal. The pod link's
extra-link declaration was never accompanied by a `persist()` call, so
`BaseGoogleLink.get_link` had no XCom payload to format; this PR wires up
`KubernetesEnginePodLink.persist(...)` in both execution paths.
>
> Sync path: the `execute_sync` override calls `super().execute_sync` in
`try/finally`, so the link still persists when the pod fails.
`KubernetesPodOperator.execute_sync` sets `self.pod` early (right after
`get_or_create_pod`), so by the time the typical failure points
(`await_pod_start`, callbacks) raise, `self.pod` is populated and the
`_persist_pod_link` guards pass.
>
> Deferrable path: persisting before `self.defer(...)` puts the XCom payload
in place before the worker hands control to the trigger. The parent
`KubernetesPodOperator` already introspects `invoke_defer_method`'s signature
and passes `context=` to overrides that accept it, so the new keyword argument
flows through end-to-end.
>
> Tests: nice coverage matrix (sync success / sync failure / sync no-pod /
deferrable / deferrable no-context), and the `attach_mock` assertion that
persist runs before defer captures the ordering constraint that matters here
(the link must be in XCom before the worker yields).
>
> A few small nice-to-haves, all non-blocking — feel free to take in a
follow-up:
>
> * `test_pod_link_url_format` reaches into the private `_format_link`. An
equivalent end-to-end test (persist → XCom → `get_link(...)`) would also catch
a rename of `_format_link` and exercise the full cycle.
> * `_persist_pod_link` short-circuits silently on every guard. A
`self.log.debug("Skipping pod link persistence because …")` on each early
return would make a future "why is the link still missing?" debug session much
faster.
> * Issue #46658 also mentions `GKEStartJobOperator`; this PR is correctly
scoped to the pod operator per the title, but `GKEStartJobOperator` has the
same shape (declares `KubernetesEngineJobLink` in `operator_extra_links`
without a matching persist call). Worth a tracking issue or a brief note in
this PR body.
>
> Approving.
>
> Drafted-by: Claude Opus 4.7 (1M context); reviewed by @potiuk before
posting
It's interesting that my agent found these testing issues blocking, and
yours didn't :D
In a second thought, it shouldn't be blocking in this case - but fixing it
would be nice (I'll give a couple days more before merging if author wants to
fix it).
--
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]
