You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "jiangzho (via GitHub)" <gi...@apache.org> on 2026/06/18 22:20:07 UTC
[PR] [SPARK-57543] Make ConfigOption dynamic override opt-in and filter dynamic config refresh [spark-kubernetes-operator]
jiangzho opened a new pull request, #713:
URL: https://github.com/apache/spark-kubernetes-operator/pull/713
### What changes were proposed in this pull request?
This PR makes runtime dynamic config override opt-in and enforces it centrally when dynamic config is refreshed:
1. `ConfigOption`
- `enableDynamicOverride` now defaults to `false` — an option must explicitly opt in via `.enableDynamicOverride(true)` to be overridable at runtime.
- Each option registers itself (keyed by its config key) into a static registry on construction, exposing `dynamicOverrideEnabledKeys()` as the allow-list of overridable keys. `enableDynamicOverride` on each option remains the single source of truth.
2. `SparkOperatorConfManager.refresh(Map)` now filters the incoming ConfigMap data against `ConfigOption.dynamicOverrideEnabledKeys()`, applying only overridable keys and logging any dropped (unknown or non-overridable) keys at WARN. `getValue(String)` and `getAll()` therefore no longer surface override values for keys that are never honored.
3. `SparkOperatorConf` options now set `enableDynamicOverride`explicitly; this also fixes docs for `LOG_CONF` and `PERIODIC_GC_INTERVAL_SECONDS`, which are only read at startup and are non-overridable.
The existing per-option check in `ConfigOption.resolveValue()` is kept as defense-in-depth.
### Why are the changes needed?
Previously `refresh()` stored the entire ConfigMap into the override map, and `enableDynamicOverride` defaulted to `true`. As a result, an override value could be stored (and surfaced via `getValue`/`getAll`/the `LOG_CONF` dump) for keys that are not meant to be hot-reloaded, and any new option was overridable by default. Defaulting to opt-in and dropping non-allow-listed keys at refresh time makes the override surface explicit, safer, and self-documenting, and avoids silently ignored config updates.
### Does this PR introduce _any_ user-facing change?
Yes. Dynamic config updates for keys that are not explicitly marked `enableDynamicOverride(true)` are now ignored (and logged at WARN) instead of being stored. In particular `spark.logConf` and `spark.kubernetes.operator.periodicGC.intervalSeconds` are no longer dynamically overridable (both are only read at startup). The generated config documentation's "Allow Hot Reloading" column reflects each option's flag.
### How was this patch tested?
Updated `ConfigOptionTest` and `SparkOperatorConfManagerTest`, and added `SparkOperatorConfManagerTest#testRefreshDropsUnknownAndNonOverridableKeys` covering the allow-list filtering. Existing `SparkOperatorConfigMapReconcilerTest` continues to exercise the dynamic-config refresh path with an overridable key.
### Was this patch authored or co-authored using generative AI tooling?
Co-Authored-By: Claude Opus 4.8
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57543] Make ConfigOption dynamic override opt-in and filter dynamic config refresh [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #713:
URL: https://github.com/apache/spark-kubernetes-operator/pull/713#issuecomment-4847802235
Gentle ping, @jiangzho .
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57543] Make ConfigOption dynamic override opt-in and filter dynamic config refresh [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #713:
URL: https://github.com/apache/spark-kubernetes-operator/pull/713#issuecomment-4887291033
Gentle ping, @jiangzho .
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57543] Make ConfigOption dynamic override opt-in and filter dynamic config refresh [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #713: [SPARK-57543] Make ConfigOption dynamic override opt-in and filter dynamic config refresh
URL: https://github.com/apache/spark-kubernetes-operator/pull/713
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57543] Make ConfigOption dynamic override opt-in and filter dynamic config refresh [spark-kubernetes-operator]
Posted by "jiangzho (via GitHub)" <gi...@apache.org>.
jiangzho commented on PR #713:
URL: https://github.com/apache/spark-kubernetes-operator/pull/713#issuecomment-4898646138
Thanks for the reminder @dongjoon-hyun - I've rebased and applied changes from review
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57543] Make ConfigOption dynamic override opt-in and filter dynamic config refresh [spark-kubernetes-operator]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #713:
URL: https://github.com/apache/spark-kubernetes-operator/pull/713#discussion_r3441941506
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/config/ConfigOption.java:
##########
@@ -34,20 +37,59 @@
/**
* Config options for Spark Operator. Supports primitive and serialized JSON.
*
+ * <p>Every option registers itself, keyed by {@link #key}, into a static registry on construction.
+ * {@link SparkOperatorConfManager#refresh(Map)} consults {@link #dynamicOverrideEnabledKeys()} to
+ * drop any dynamic-config update that targets an unknown key or one whose {@link
+ * #enableDynamicOverride} is {@code false}.
+ *
* @param <T> The type of the config option's value.
*/
-@RequiredArgsConstructor
-@AllArgsConstructor
@EqualsAndHashCode
@ToString
-@Builder
@Slf4j
public class ConfigOption<T> {
- @Getter @Builder.Default private final boolean enableDynamicOverride = true;
- @Getter private String key;
- @Getter private String description;
- @Getter private T defaultValue;
- @Getter private Class<T> typeParameterClass;
+ /** Indexes every declared option so dynamic-config refresh can check overridability. */
+ private static final Map<String, ConfigOption<?>> REGISTRY = new ConcurrentHashMap<>();
+
+ /**
+ * Whether this option may be overridden at runtime via dynamic config. Defaults to {@code false}
+ * (opt-in): dynamic override must be explicitly enabled through the builder.
+ */
+ @Getter private final boolean enableDynamicOverride;
+ @Getter private final String key;
+ @Getter private final String description;
+ @Getter private final T defaultValue;
+ @Getter private final Class<T> typeParameterClass;
+
+ @Builder
+ ConfigOption(
+ boolean enableDynamicOverride,
+ String key,
+ String description,
+ T defaultValue,
+ Class<T> typeParameterClass) {
+ this.enableDynamicOverride = enableDynamicOverride;
+ this.key = key;
+ this.description = description;
+ this.defaultValue = defaultValue;
+ this.typeParameterClass = typeParameterClass;
+ if (StringUtils.isNotEmpty(key)) {
+ REGISTRY.put(key, this);
+ }
+ }
+
+ /**
+ * Returns the keys of all declared options that permit runtime dynamic override. Used as the
+ * allow-list when refreshing dynamic config.
+ *
+ * @return An immutable set of dynamically overridable config keys.
+ */
+ public static Set<String> dynamicOverrideEnabledKeys() {
Review Comment:
This allow-list reflects only `ConfigOption`s that have already been *constructed*. Because the real options are `static final` fields on `SparkOperatorConf`, they register themselves only when that class is first initialized — so `refresh()`'s correctness rests on an implicit invariant: `SparkOperatorConf` must be class-loaded before any `refresh()` runs.
Today that holds, but only incidentally: `SparkOperator` reads `SparkOperatorConf.LOG_CONF` and `SparkOperatorConf.DYNAMIC_CONFIG_ENABLED` during boot (`SparkOperator.java:104` and `:109`), which forces class-init before `registerSparkOperatorConfMonitor()` wires up the ConfigMap reconciler that calls `refresh()`. If that ordering ever changes — a new entrypoint, a reordering of the boot sequence, or a test that drives `refresh()` without touching `SparkOperatorConf` — `dynamicOverrideEnabledKeys()` would return an empty (or partial) set and **every** incoming override would be silently dropped, with only a WARN to show for it.
Suggest making the invariant explicit rather than relying on boot order. Minimally, a Javadoc note here stating that the returned set only includes options constructed so far. Better, have the conf manager force initialization before its first refresh, e.g. reference `SparkOperatorConf` (a class touch is enough to trigger static init) when the manager starts, so the allow-list is guaranteed complete independent of who calls `refresh()` first.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org