gerkElznik commented on PR #1126: URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#issuecomment-4938361057
Thanks @Dennis-Mircea! Inline comments are all addressed in the latest commits on both PRs (including the Chinese docs sync). Before responding to the cleanup proposal I tried each piece in a kind cluster and traced the code paths, to check my own understanding. Taking your three items in order: **1. Dropping the SLF4J reporter and interval overrides from values.yaml** Confirmed the compiled defaults are 60s (`kubernetes.operator.reconcile.interval`) and 10s (`kubernetes.operator.observer.progress-check.interval`), and that both are currently dead in Helm installs since the chart always overrides them. Dropping the overrides without touching the compiled defaults does change default-install behavior though: I measured the reconcile cadence going from 15.0s to 60.1s, and the progress check would go from 5s to 10s (slower observation of in-progress deployments and savepoints). So the choice is between changing the compiled defaults to 15s/5s (which also affects non-Helm deployments, the generated docs, and idle load on the API server and job REST endpoints) or letting default installs slow down with a release note. I don't have a strong opinion on which, but it feels like a decision that deserves its own visibility. One consistency note: dropping the SLF4J reporter is also a behavior change on default installs, since operator metrics stop being logged every 5 minutes and anyone scraping or alerting on those lines would notice. I agree it should be user choice going forward; I'd just put it in the same release note. **2. Migrating the Java-17 opts to compiled defaults** I think this is the right destination, and it's the same end-state note 4 above was gesturing at. Two design questions the follow-up would need to answer, since the per-version keys are dynamic string prefixes resolved at runtime (`FlinkConfigManager.getRelevantVersionPrefixes`) and can't be plain `ConfigOption` defaults: - Where the compiled seed gets injected decides precedence; user config needs to keep winning over it. - Today `defaultConfiguration.append: false` produces a config without the Java-17 opts entirely. Compiled defaults can't be shed by omitting text, so the migration probably needs an explicit off-switch to preserve that escape hatch. **3. watchNamespaces and operatorHealth as env vars** `WATCH_NAMESPACES` works on the operator side exactly as you said: I set it on the operator container in kind and the watch scoped correctly with no code change. Two wrinkles I found while tracing the full path: - The webhook consumes the watched namespaces too (`FlinkOperatorWebhook` scopes its informers through the same config path), and it runs as a separate container with its own env block. The chart would need to set the var on both containers; otherwise the webhook informers stay cluster-scoped, which fails under the namespaced RBAC that setting `watchNamespaces` switches on. - The env var takes precedence over the config file, so shipping namespaces via env silently disables `kubernetes.operator.dynamic.namespaces.enabled` (no-restart namespace changes via ConfigMap edit). That might be acceptable, since an env change through helm upgrade forces a rolling restart, which is arguably more correct than today's silently stale config. But it seems worth a deliberate call in the follow-up. For the health probe: nothing reads `KUBERNETES_OPERATOR_HEALTH_PROBE_*` today. Those names don't exist in the tree (I checked `EnvUtils` and `OperatorHealthService`), so that half is a small amount of new operator code mirroring `ENV_WATCH_NAMESPACES`. Notably it's only needed to keep `operatorHealth.port` overrides and disablement working: for a default install the compiled defaults (true/8085) already match what the chart writes, so dropping those two ConfigMap lines is behavior-neutral there. The Helm value itself stays either way, since it also wires the containerPort and the liveness/startup probes. **On sequencing** Landing #1152 first makes sense to me regardless. For this PR, my thinking is that the cleanup and the fix here are complementary rather than alternatives: the cleanup removes the collision surface, but even with an empty seed and env vars the chart still needs this PR's template changes to emit and mount the user's `config.yaml` (the always-true `hasKey` is what makes that branch unreachable today). That makes me lean toward landing this PR at its current scope and doing the cleanup as a dedicated follow-up ticket, where the compiled-defaults question can get proper visibility. Happy to fold it into this PR instead if you and @gyfora prefer; if it goes the follow-up route, I'm glad to file the ticket and take it on. -- 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]
