-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Support PR: Add TTL-enabled LRU cache for StatsD metrics aggregation #60933
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support PR: Add TTL-enabled LRU cache for StatsD metrics aggregation #60933
Conversation
…d user defined args as well
…values.schema.json
…ated schema.json properly
Co-authored-by: Przemysław Mirowski <miretpl@gmail.com>
Co-authored-by: Przemysław Mirowski <miretpl@gmail.com>
00a45b7 to
657381d
Compare
|
Thanks @AutomationDev85 for moving this forward. Yes the PR is pending since a while. But you also seem to be un-lucky... can you fix the tests? |
657381d to
846ca6e
Compare
846ca6e to
18e55b6
Compare
|
@jscheffl Thanks for the response. I fixed this issue. Now it looks good. |
jscheffl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing, then we can put this into 1.19.0 and close this improvement pending since a long time! Cool!
…pache#60933) * Making statsd-exporter TTL & cache-size/type configurable in Airflow Helm chart * Added statsd configs in values.schema.json * Fixing test errors * updated the default values for statsd in schema.json * Added default argumentd as cache sieze cache type and ttl, accomodated user defined args as well * Removed EOF error from statsd deployment and improved description in values.schema.json * Added default values in deployment.yaml, removed spelling errors formated schema.json properly * added newlines and changed 'lru' to `lru` * edited as per test output * Making changes in deployment file * Added newsfragments for statsd changes * removed 0s from deployment.yaml * resolving errors in newsfragment, and tests * Patch errrors related to test cases * removed errors from test_statsd and test_apiserver * removed default args for statsd * added configs in default and removed overridden confugs * removed defaults for statsd * removed default args to resolve the test * removed syntax error * Added values in render_chart * removed unnecessary issues * helm unit test * Moved args into else block * Revusuted statsd args and made changes * solved resources error * Changed JSON for values schema * resolved error in the statsd tests * updated the deployment and values.yaml * resolving ci formatting issues * Update chart/newsfragments/51792.significant.rst Co-authored-by: Przemysław Mirowski <miretpl@gmail.com> * Update chart/newsfragments/51792.significant.rst Co-authored-by: Przemysław Mirowski <miretpl@gmail.com> * restructured cache options in statsd * fixing indentation in deployment.yaml * fix: updated newsfragment file and fixed CI static check * updated the indentations in values and statsd deployment * reverted the apiserver changes * changed the formatting * Remove TTL parameter from command line and add into the defaults part of config map --------- Co-authored-by: shubham36deshpande <shubham36deshpande@gmail.com> Co-authored-by: shubham36deshpande <116251650+shubham36deshpande@users.noreply.github.com> Co-authored-by: Przemysław Mirowski <miretpl@gmail.com> Co-authored-by: AutomationDev85 <AutomationDev85>
Overview
We are eagerly looking forward to seeing this PR merged #51792 from @shubham36deshpande. We would like to help bring this PR over the finish line, as we are currently facing this exact issue in our deployment and need to use it. As we do not have access to his repo we opened a new PR.
We noticed @jscheffl comment in the PR regarding the missing TTL parameter and found this parameter in the defaults section of the config mapping. In response, we have added a commit to the branch that includes the parameter in the config mapping of the StatsD deployment.
Thanks @shubham36deshpande for finding this topic.