RehanAhmad25 commented on PR #69925: URL: https://github.com/apache/airflow/pull/69925#issuecomment-5278891824
> This and #69746 fix the same thing in different ways. We should first figure out the correct approach in the issue. Agreed, let's settle this on the issue so it's not split across two threads. For what it's worth, my read on the tradeoff: #69746's TimeOfDayTrigger genuinely fixes more than mine does, it caught and fixed the class-level `start_trigger_args` mutation bug independently, which mine only sidesteps (the buggy code path becomes dead code once `start_from_trigger` is deprecated, but I didn't find that bug myself). That's a real point in its favor. My case for the simpler deprecation approach: `start_from_trigger` avoids one worker-slot poke/defer cycle by starting the sensor directly on the triggerer, a fairly narrow optimization. Preserving it costs a new trigger class, lazy-resolution logic, and DST edge case handling (spring-forward gaps, fall-back ambiguity) that's now permanent surface area to maintain and could itself grow bugs over time. Given how narrow the benefit is, I think removing it is proportionate, but I'm not tied to that if the consensus lands the other way, happy to adapt or step back from this fix if #69746 is the preferred direction. I'll post this same comparison on the issue. -- 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]
