Myasuka commented on code in PR #28732:
URL: https://github.com/apache/flink/pull/28732#discussion_r3702568565
##########
flink-core/src/main/java/org/apache/flink/configuration/ClusterOptions.java:
##########
@@ -140,6 +140,31 @@ public class ClusterOptions {
.withDescription(
"The maximum stacktrace depth of TaskManager and
JobManager's thread dump web-frontend displayed.");
+ @Documentation.Section(Documentation.Sections.EXPERT_CLUSTER)
+ public static final ConfigOption<String> THREAD_DUMP_DEFAULT_MODE =
Review Comment:
`THREAD_DUMP_DEFAULT_MODE` is declared as `.stringType()` with a default of
`"FULL"`, and the actual parsing happens later in
`ThreadDumpMode.fromStringOrDefault`, which silently falls back to `FULL` for
any null/blank/unrecognized value.
That fallback behavior is risky specifically for this option: if an operator
mistypes the value (e.g. `"Ltie"`), the config loads without error and the
operator has no indication their setting was ignored — the cluster silently
keeps using `FULL`, the exact high-risk mode this PR is meant to let people opt
out of.
Could we either:
1. Switch this to `ConfigOptions.key(...).enumType(ThreadDumpMode.class)` so
invalid values fail fast at config-load time instead of being silently
swallowed, or
2. If we want to keep it as a free-form string (e.g. for forward
compatibility), at least log a `WARN` in `fromStringOrDefault`/`resolve` when
the configured value doesn't match a known `ThreadDumpMode`, so this
misconfiguration is visible in the logs.
Option 1 seems preferable given `ThreadDumpMode` is a fixed, closed set of
values.
--
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]
