PavelZeger commented on PR #1116:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/1116#issuecomment-4524846390
> The framing of this PR is wrong: A `LOG.warn(msg)` without the `Throwable`
is not automatically a bug, in several of the call sites you are changing it is
deliberate. Adding the stack trace blanket-style produces log spam on expected
paths and obscures the intent of the original code. Each call site needs to be
evaluated on its own merits.
>
> Going site by site:
>
> * **`KubernetesAutoScalerStateStore#decompress` (line 394)** - this is a
known fallback path. The comment one line below literally says `// Fall back to
non-compressed for migration`. During migration this catch will fire **on every
annotation read** for resources stored under the old format. Today it logs one
tidy `WARN`, after this PR it logs a full stack trace per annotation, which is
exactly the kind of noise the original author was avoiding. Please drop this
change.
> * **`MemoryTuning#tune` (line 91)** - `IllegalConfigurationException` here
is a user-config error. The user needs the message ("memory configuration is
not valid") as it is already clear enough, not a parser stack trace from
`MemoryProcessSpec`. Adding the throwable here adds 30+ stack frames of Flink
internals on every reconcile while the user has invalid memory config.
> * **`FlinkConfigManager` (line 265 & 270)** - same category as above, two
catches for `NumberFormatException` / `IllegalArgumentException` on **user
config keys**. The message already names the offending key. The exception type
is the message essentially. A stack trace adds nothing diagnostic for a parse
failure on a known string.
> * **`IngressUtils#isVersionAtLeast` (line 370)** - catches
`IllegalArgumentException` from `ModuleDescriptor.Version.parse`. The message
already prints the unparseable `serverVersion` string.
> * **`StateSnapshotReconciler` (line 177)** - this one is justified.
Savepoint disposal failures are real operational failures where the upstream
exception (Flink REST client, IO, etc.) is exactly what an operator needs to
debug. This is the only change that I'd keep.
>
> So out of 5 sites, 1 is clearly correct, 1 is clearly wrong (the
decompression fallback), and 3 are noise. Please split the savepoint disposal
fix into its own small PR with a focused JIRA, and drop the rest unless you can
show a concrete debugging scenario each one improves.
>
> The PR description has the same formatting issue as #1115.
Thanks for the review @Dennis-Mircea .
All required fixes were addressed and the PR format was applied
--
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]
