dheerajturaga commented on PR #70690:
URL: https://github.com/apache/airflow/pull/70690#issuecomment-5123091405
Thanks for the careful review — all four are fair. Pushed fixes for three;
the fourth is a design question I'd like your steer on.
**Version gate (`resolve_mp_start_method`).** Fixed. You're right that
gating on 3.3+ silently dropped the `mp_start_method` config on 2.11–3.2 and
made the `provider.yaml` text false there. Now reads `conf.get("celery", ...)
or conf.get("core", ...)` with `fallback=None` directly, so the escape hatch
works on every supported version.
**forkserver with no preload / fresh pool per `sync()`.** Fixed by making
the pool persistent on `BulkStateFetcher` — built lazily, reused across syncs,
shut down in `end()`/`terminate()`, and recreated if a worker death breaks it.
One thing worth flagging: in my measurements `set_forkserver_preload` alone did
*not* amortize when a fresh pool was built each sync — each worker still
re-imported (I confirmed via a per-worker PID marker that the preloaded module
wasn't inherited). Hoisting the pool out of the per-`sync()` scope is what
actually removes the cost, so I went with that. Happy to be shown otherwise if
you've seen preload help across distinct pools.
**Stale docstring.** Fixed.
**Bounding publish time per heartbeat.** This is the one I'd like your call
on. You're right that the batch tracks `[core] parallelism` (default 32) via
`open_slots`, not `max_tis_per_query`, so an unresponsive broker gives
~`parallelism * operation_timeout` ≈ 32s serial, over the 30s health threshold,
with `task_publish_max_retries` re-queuing the timed-out keys. I confirmed that
returning only a published prefix is safe: `_get_workloads_to_schedule` only
reads `queued_tasks`, and the pop happens per-returned-result in
`_send_workloads`, so an un-sent remainder stays queued and is retried next
heartbeat without consuming slots. My inclination is a wall-clock publish
budget checked between sends (each send is already bounded by its own
`operation_timeout` alarm), with the remainder deferred — defaulting to
something like 15s so healthy brokers never hit it. Would you prefer that, a
simpler per-heartbeat count cap, or just documenting the tradeoff? And if the
budget, do you wan
t it protective-by-default or opt-in?
---
Drafted-by: Claude Code (Opus 5); reviewed by @dheerajturaga 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]
