potiuk commented on PR #66400: URL: https://github.com/apache/airflow/pull/66400#issuecomment-4381947653
Pushed `69070f0e9c` to address point (1) — `_alive_other_scheduler_job_ids` now feeds the scalar cursor straight into a set comprehension, no intermediate `list[int]` materialised. On the other concerns: **`FOR UPDATE … SKIP LOCKED`** — I don't think it fits here. The query is a pure read of "who is alive right now" and is racing against every other scheduler's heartbeat write on its own `Job` row. With `FOR UPDATE` we'd block on those heartbeats; with `SKIP LOCKED` we'd routinely *miss* siblings whose row is in the middle of a heartbeat update and falsely classify them as dead — which would relabel their pods and bring the thrashing back. So adding the lock would actively make the selector wrong rather than safer. The race you're describing — two healthy schedulers both adopting the same dead-scheduler pod at the same kube-API tick — is real, but it's at the K8s layer, not at the DB. Worst case: scheduler A patches the pod with `airflow-worker=A`, scheduler B patches it with `airflow-worker=B`, last write wins, the winning scheduler's watcher deletes the pod. One redundant `patch_namespaced_pod` call per pod in that exact-collision window — not a correctness problem and orders of magnitude smaller than the per-tick thrash this PR fixes. Truly serialising adoption between alive schedulers would need K8s-level coordination (e.g. a Lease-based leader election around `_adopt_completed_pods`), which is well beyond the scope of fixing the regression #61839 introduced. Happy to file a follow-up issue if you want to pursue that. **Limit / batching** — the K8s pod-list label selector goes in the URL query string. With a generous 100 alive schedulers (typical HA runs 2–5; >10 is already unusual) the resulting `notin (…)` clause is ~1.5 KB; the kube-apiserver URL-length ceiling is in the hundreds-of-KB range and `metav1.LabelSelector` itself doesn't cap `MatchExpressions` value cardinality. So at any operationally plausible scheduler count we're 100×–1000× under the limit and don't need to batch the K8s side. If you'd still like a defensive cap with a logged warning above some threshold (say 64 alive siblings) I can add that — it'd be a sanity guard rather than a correctness fix. --- Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting -- 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]
