bkinzle commented on PR #1126: URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#issuecomment-4921407192
Thanks for the feedback @Dennis-Mircea! Inline comments are all addressed in the latest commits on both PRs (including the Chinese docs sync). On the broader cleanup, I like the destination a lot, and I think it's the same end-state my note (4) above was gesturing at. `config.yaml` containing only user-supplied content seems like the right place for this to eventually land. Before forming an opinion on where the work should happen, I tried the pieces out in a kind cluster to check my own understanding, and I'd summarize what I saw as: - `WATCH_NAMESPACES` works exactly as you said: setting the env on the operator deployment scoped the watch with no operator change needed; the chart just doesn't set it today. The health-probe side looked different in my test though; nothing seems to read the `KUBERNETES_OPERATOR_HEALTH_PROBE_*` env vars currently (I set them and the operator kept `true`/`8085` from the file), so I suspect that half would need a small amount of new operator code. I may be missing something though. - Dropping the values.yaml overrides without touching the compiled defaults does change behavior -- I measured the reconcile cadence going from 15.0s to 60.1s on a default install. So if I'm reading it right, either the compiled defaults change to 15s/5s (which would also affect non-Helm users), or default installs slow down 4x and probably want a release note. I'm not sure which is the better call, which is partly why I wonder if it deserves its own ticket where that tradeoff can get proper visibility. - For the Java-17 opts migration I'm less sure of the shape... it seems to involve deciding where per-Flink-version defaults live in operator code, and I don't have a strong sense yet of the right home for that. The way I see it there are two options: 1. Keep this PR at its current scope (it fixes the user-facing bug on its own), and do the cleanup as a dedicated follow-up ticket implementing what you describe. 2. Fold the cleanup into this PR and reshape it around the broader fix. I'll defer to your and @gyfora's call on which path to take. 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]
