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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
