You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/01/25 18:21:54 UTC
[GitHub] [airflow] viktorvia opened a new pull request #21106: Fix StatD timing metric units
viktorvia opened a new pull request #21106:
URL: https://github.com/apache/airflow/pull/21106
Two StatD timing metrics were calculated in seconds and given
directly to StatD which accepts either milliseconds when
passed directly or timedelta.
Switched the two to timedelta and updated unit tests.
closes: #20804
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #21106: Fix StatD timing metric units
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #21106:
URL: https://github.com/apache/airflow/pull/21106#issuecomment-1021814946
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`).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] viktorvia commented on pull request #21106: Fix StatD timing metric units
Posted by GitBox <gi...@apache.org>.
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #21106: Fix StatD timing metric units
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #21106:
URL: https://github.com/apache/airflow/pull/21106#issuecomment-1021479201
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org