gingeekrishna commented on PR #69102: URL: https://github.com/apache/airflow/pull/69102#issuecomment-4828569085
Thanks for the review, @SameerMesiah97 — good questions. Let me address both: **1. Worker slot occupancy** The polling does not add any extra occupancy time. The worker was _already_ occupied for the full duration of the launcher subprocess run before my change — `run_beam_command` has always blocked on `select.select(reads, [], [], 5)` in a loop until the subprocess exits. My `periodic_callback` runs _inside that same existing loop_, within the 5-second `select` timeout that was already there. If anything, this fix can only _reduce_ occupancy: when the job ID is resolved via the Dataflow API before the launcher finishes printing it to stdout, `run_beam_command` returns early (see line 204) and the operator can defer sooner. **2. Generic callback vs. dedicated waiter** The `periodic_callback` is intentionally generic because `run_beam_command` lives in the _Beam_ hook, which should not know about the Dataflow API. A "dedicated waiter" would either need to couple Dataflow API knowledge into that generic hook, or be a wrapper class around `run_beam_command` that ends up doing the same thing less directly. The callback pattern is already the established idiom here — `process_line_callback` and `is_dataflow_job_id_exist_callback` both pre-existed my change and follow the same shape. `periodic_callback` is just a timed variant of the same mechanism. Happy to rename it or restructure if you have a specific shape in mind for the dedicated helper. -- 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]
