[GitHub] [airflow] dstandish commented on a change in pull request #13209: Add support for worker persistence with KEDA v2.0.0 in helm chart

[GitBox <gi...@apache.org>]

dstandish commented on a change in pull request #13209:
URL: https://github.com/apache/airflow/pull/13209#discussion_r588960938



##########
File path: chart/tests/helm_template_generator.py
##########
@@ -61,7 +88,7 @@ def validate_k8s_object(instance):
     validate.validate(instance)
 
 
-def render_chart(name="RELEASE-NAME", values=None, show_only=None, validate_schema=True):

Review comment:
       `validate_schema` was added recently to allow tests for CRDs but with this PR we have a way of pulling in the definition.
   it does require a network call here fwiw: https://github.com/apache/airflow/pull/13209/files#diff-91361141bde70cbfc90142ebe2377ce918025d3343d9216e00e50c66f083b12cR65

##########
File path: chart/templates/workers/worker-kedaautoscaler.yaml
##########
@@ -35,14 +35,18 @@ metadata:
 {{- end }}
 spec:
   scaleTargetRef:
-    deploymentName: {{ .Release.Name }}-worker
+    kind: {{ ternary "StatefulSet" "Deployment" .Values.workers.persistence.enabled }}
+    name: {{ .Release.Name }}-worker
   pollingInterval:  {{ .Values.workers.keda.pollingInterval }}   # Optional. Default: 30 seconds
   cooldownPeriod: {{ .Values.workers.keda.cooldownPeriod }}    # Optional. Default: 300 seconds
   maxReplicaCount: {{ .Values.workers.keda.maxReplicaCount }}   # Optional. Default: 100
   triggers:
     - type: postgresql
       metadata:
         targetQueryValue: "1"
-        connection: AIRFLOW_CONN_AIRFLOW_DB
-        query: "SELECT ceil(COUNT(*)::decimal / {{ .Values.config.celery.worker_concurrency }}) FROM task_instance WHERE state='running' OR state='queued'"

Review comment:
       while updating `connectionFromEnv` just decided to make this a bit more readable by splitting onto separate lines

##########
File path: docs/helm-chart/keda.rst
##########
@@ -24,27 +24,23 @@ KEDA stands for Kubernetes Event Driven Autoscaling.
 `KEDA <https://github.com/kedacore/keda>`__ is a custom controller that
 allows users to create custom bindings to the Kubernetes `Horizontal Pod
 Autoscaler <https://kubernetes.io/docs/tasks/run-application/horizontal-pod-autoscale/>`__.
-We have built scalers that allows users to create scalers based on

Review comment:
       i removed this sentence because if i recall correctly the "we" is astronomer, so in this repo it doesn't really have the same meaning and i don't think the statement belongs.  but it's also sortof beside the point.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org