jedcunningham commented on code in PR #48032:
URL: https://github.com/apache/airflow/pull/48032#discussion_r2008347291
##########
chart/values.yaml:
##########
@@ -1805,8 +1805,11 @@ triggerer:
# periodSeconds: 15
# Query to use for KEDA autoscaling. Must return a single integer.
+ # PR 48032 renames default_capacity to capacity for Airflow 3.0. However,
this chart will be used for
+ # both Airflow 2.X and 3.0. Here, we're updating the query to point to
either 1) capacity,
+ # 2) default_capacity, or 3) 1000.
Review Comment:
```suggestion
```
Let's do a similar comment in the helper instead.
##########
airflow-core/src/airflow/jobs/triggerer_job_runner.py:
##########
@@ -95,7 +95,11 @@ def __init__(
):
super().__init__(job)
if capacity is None:
- self.capacity = conf.getint("triggerer", "default_capacity",
fallback=1000)
+ # PR 48032 renames the triggerer default_capacity to capacity. The
precedence is capacity ->
+ # default_capacity -> 1000
+ self.capacity = conf.getint("triggerer", "capacity") or
conf.getint(
+ "triggerer", "default_capacity", default=1000
+ )
Review Comment:
```suggestion
self.capacity = conf.getint("triggerer", "capacity")
```
This is all you need. Your addition to deprecated_options will fall back to
the old config option or the default, if not set explicitly.
##########
chart/templates/_helpers.yaml:
##########
@@ -664,6 +664,17 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{- include "_serviceAccountName" (merge (dict "key" "triggerer") .) -}}
{{- end }}
+{{/* Set the triggerer capacity to use */}}
Review Comment:
```suggestion
{{/* The triggerer capacity to use, accounting for the config option rename
between Airflow 2 and 3 */}}
```
##########
chart/values.yaml:
##########
@@ -2710,8 +2713,12 @@ config:
worker_container_repository: '{{ .Values.images.airflow.repository |
default .Values.defaultAirflowRepository }}'
worker_container_tag: '{{ .Values.images.airflow.tag | default
.Values.defaultAirflowTag }}'
multi_namespace_mode: '{{ ternary "True" "False"
.Values.multiNamespaceMode }}'
+
+ # PR 48032 renames triggerer.default_capacity to capacity. However, as this
chart is used for both Airflow 3.0
+ # and 2.X, only the default_capacity is provided
triggerer:
default_capacity: 1000
Review Comment:
```suggestion
```
We can axe this too - 1000 is the default in 2 already.
--
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]
