aeroyorch commented on PR #69774: URL: https://github.com/apache/airflow/pull/69774#issuecomment-5269527373
> LGTM and Thanks for the improvement! I actually just noticed this issue myself, and was glad to see someone was already working on a fix! > > Just a small idea, feel free to take it or leave it if it doesn't quite fit! > > Would it make sense to move the warning logs into `DBDagBag.__init__` itself? To fully close out #69001, this change will eventually need to be wired up to the **Scheduler** too , and at that point we'd want to warn users about the exact same negative-value cases again. If the warning lived inside `DBDagBag.__init__` from the start, it'd be a lot easier to keep things consistent as more callers get hooked up down the road , one place to maintain, and no risk of some future caller quietly forgetting to warn. > > The trade-off is we'd lose the ability to tell the user exactly which config option was the problem (e.g. `[api] dag_cache_size` vs. a future `[scheduler] dag_bag_cache_size`) Thanks for the review! Fair point, but I'd rather leave it as is for now, the trade-off you mention is the deciding one for me: keeping it at the call site lets us name the actual config option that's wrong, which is what the user needs to fix. Happy to revisit when the `Scheduler` part lands for #69001. -- 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]
