Miretpl commented on code in PR #51792:
URL: https://github.com/apache/airflow/pull/51792#discussion_r2603761787
##########
chart/newsfragments/51792.significant.rst:
##########
@@ -0,0 +1,11 @@
+StatsD metrics aggregation now supports configurable TTL-enabled LRU cache to
prevent memory growth in long-running daemons.
+
+The Helm chart now includes new configuration options for StatsD aggregation
management:
+
+* ``statsd.cacheType`` - Enable TTL-enabled ``lru`` cache or ``random`` cache
for metrics aggregation (default: ``lru``)
+* ``statsd.cacheSize`` - Maximum number of metrics to cache (default: ``1000``)
+* ``statsd.ttl`` - Time-to-live for cached metrics in seconds (``0s`` is TTL
disabled) (default: ``0s``)
+
+This feature addresses uncontrolled memory growth in StatsD daemons by
automatically cleaning up stale or unused metric entries. When enabled, the
cache uses both LRU (Least Recently Used) eviction and TTL (Time To Live)
expiration to manage memory usage effectively.
+
+To maintain backward compatibility, the values are kept default values from
``StatsD`` Users experiencing memory growth issues with StatsD can enable this
feature by setting ``statsd.cache: true`` in their Helm values.
Review Comment:
```suggestion
To maintain backward compatibility, the default behaviour remains unchanged.
Users experiencing memory growth issues with StatsD can enable this feature by
setting ``statsd.cache: true`` in their Helm values.
```
##########
chart/values.schema.json:
##########
@@ -5639,47 +5654,11 @@
},
"value": {
"type": "string"
- },
- "valueFrom": {
- "type": "object",
- "properties": {
- "configMapKeyRef": {
- "$ref":
"#/definitions/io.k8s.api.core.v1.ConfigMapKeySelector",
- "description": "Selects a key of a
ConfigMap."
- },
- "secretKeyRef": {
- "$ref":
"#/definitions/io.k8s.api.core.v1.SecretKeySelector",
- "description": "Selects a key of a
secret in the pod's namespace"
- }
- },
- "anyOf": [
- {
- "required": [
- "configMapKeyRef"
- ]
- },
- {
- "required": [
- "secretKeyRef"
- ]
- }
- ]
}
},
"required": [
- "name"
- ],
- "anyOf": [
- {
- "required": [
- "value"
- ]
- },
- {
- "required": [
- "valueFrom"
- ]
- }
+ "name",
+ "value"
Review Comment:
Unrelated changes.
##########
chart/newsfragments/51792.significant.rst:
##########
@@ -0,0 +1,11 @@
+StatsD metrics aggregation now supports configurable TTL-enabled LRU cache to
prevent memory growth in long-running daemons.
+
+The Helm chart now includes new configuration options for StatsD aggregation
management:
Review Comment:
```suggestion
The Helm Chart now includes new configuration options for StatsD aggregation
management:
```
##########
chart/values.schema.json:
##########
@@ -7095,6 +7074,21 @@
"type": "boolean",
"default": true
},
+ "cacheSize": {
+ "description": "Maximum number of metric mappings to cache
in memory. Higher values improve performance for frequently used metrics but
consume more memory.",
+ "type": "integer",
+ "default": 1000
+ },
+ "cacheType": {
+ "description": "Cache eviction strategy for metric
mappings. `lru` (Least Recently Used) evicts oldest accessed items, 'random'
evicts randomly selected items.",
+ "type": "string",
+ "default": "lru"
+ },
+ "ttl": {
+ "description": "Time-to-live for cached metric mappings.
Determines how long mappings remain in cache before expiring. Set to '0s' to
disable expiration.",
+ "type": "string",
+ "default": "0s"
+ },
Review Comment:
There is missing `statsd.cache` field which is mentioned in newsfragment.
Also, maybe it would be a good idea to place cache related options under
`statsd.cache` section with `enabled`, `size`, `type` and `ttl` options?
##########
chart/values.schema.json:
##########
@@ -480,6 +480,21 @@
"type": "boolean",
"default": false
},
+ "cacheSize": {
+ "description": "Maximum number of metric mappings
to cache in memory. Higher values improve performance for frequently used
metrics but consume more memory.",
+ "type": "integer",
+ "default": 1000
+ },
+ "cacheType": {
+ "description": "Cache eviction strategy for metric
mappings. `lru` (Least Recently Used) evicts oldest accessed items, 'random'
evicts randomly selected items.",
+ "type": "string",
+ "default": "lru"
+ },
+ "ttl": {
+ "description": "Time-to-live for cached metric
mappings. Determines how long mappings remain in cache before expiring. Set to
'0s' to disable expiration.",
+ "type": "string",
+ "default": "0s"
+ },
Review Comment:
These changes are under `ingress` section, which is rather not related.
##########
chart/values.schema.json:
##########
@@ -5639,47 +5654,11 @@
},
"value": {
"type": "string"
- },
- "valueFrom": {
- "type": "object",
- "properties": {
- "configMapKeyRef": {
- "$ref":
"#/definitions/io.k8s.api.core.v1.ConfigMapKeySelector",
- "description": "Selects a key of a
ConfigMap."
- },
- "secretKeyRef": {
- "$ref":
"#/definitions/io.k8s.api.core.v1.SecretKeySelector",
- "description": "Selects a key of a
secret in the pod's namespace"
- }
- },
- "anyOf": [
- {
- "required": [
- "configMapKeyRef"
- ]
- },
- {
- "required": [
- "secretKeyRef"
- ]
- }
- ]
Review Comment:
Unrelated changes.
--
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]
