jscheffl commented on code in PR #65852:
URL: https://github.com/apache/airflow/pull/65852#discussion_r3143883958
##########
chart/templates/workers/worker-deployment.yaml:
##########
@@ -348,7 +348,7 @@ spec:
{{- if and .Values.dags.gitSync.enabled (not
.Values.dags.persistence.enabled) }}
{{- include "git_sync_container" . | nindent 8 }}
{{- end }}
- {{- if and $persistence .Values.workers.logGroomerSidecar.enabled }}
+ {{- if .Values.workers.logGroomerSidecar.enabled }}
Review Comment:
Mhm, I am not really (fully) agreeing to Jarek's feedback here. I think
comparing both solutions I think adding another parameter just to 100% keep
backcompat is a bit too much.
Actually I'd see the (minimal) behavior change rather as a bug fix. Because
if you disable persistence and "forget" to enable log groomer you will run out
of storage anyway in a short period of work. Changing it like in this PR
removes a standard operational problem that in my view weights larger than
people wondering why logs have been removed (before the disk ran out of
storage). Still it is configurable if you want to turn it off (to have your
disk running full).
Adding another parameter just adds complexity. So I'd prefer this PR over
the alternative. If @potiuk worries about semver and back-compat in the case of
non persistence I'd rather make a newsfragment to highlight the (minimal)
behavior change.
--
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]
