viktorvia commented on pull request #21106: URL: https://github.com/apache/airflow/pull/21106#issuecomment-1021962829
> Changing `DagFileStat` has some consequences, at least you need to also fix other parts of `DagFileProcessorManager` to accomodate the change. It’s not undoable, but I feel it’d be easier (much smaller scope) to add add `* 1000` to the `timing` hook on `SafeStatsdLogger` (and maybe `SafeDogStatsdLogger`). Agree but in this lucky case the scope is very limited since `DagFileStat` is only ever used by `DagFileProcessorManager` in its `_file_stats` dictionary. Furthermore I couldn't find a single leak of this `_file_stats` dictionary. Your question got me to run a grep though and I've found a unit test that initializes `DagFileStat` the old way and I've updated that as well. The test was passing though. The only other way out for `last_duration` is through `get_last_runtime()` which continues to return seconds as before. And even that is not used by anything else except `DagFileProcessorManager`. I've updated the unit test that involves it (it did broke in the expected way). I think we're safe? Should I add some more unit tests? -- 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]
