Vamsi-klu commented on PR #69746: URL: https://github.com/apache/airflow/pull/69746#issuecomment-5149823773
You were right that removal wasn't the only option — I ended up building the lazy version you suggested. start_from_trigger stays, so no major bump: there's a new TimeOfDayTrigger that stores the wall-clock time plus timezone and only resolves the concrete UTC moment when the trigger actually starts, then hands off to DateTimeTrigger's existing wait loop. start_trigger_args is built per instance now, which also kills the shared-class mutation bug. There are tests that serialize the same Dag under two different frozen clocks and expect byte-identical payloads, so the hash churn can't quietly come back. The earlier inline comments (the kwargs.pop signature, recomputed target_datetime, the getattr UTC fallback) were all about the removal version — that code doesn't exist anymore. The latest push also fixes the three CI failures that were mine: an RST error in the new trigger docstring that broke the docs build, the pendulum in_timezone typing, and the serialization tests now pick whichever API exists on the running Airflow (2.11 through main) instead of assuming LazyDeserializedDAG.from_dag. On DST, the semantics I went with: a target time inside the spring-forward gap shifts to the next valid wall time, an ambiguous fall-back time takes the first occurrence, and "already passed today" still fires today rather than rolling to tomorrow. All documented and tested, but if maintainers want different behavior I'm fine changing it. A second pair of eyes on the trigger serialization would be welcome. --- Drafted-by: Claude Code (Fable 5); reviewed by @Vamsi-klu 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]
