You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@spark.apache.org by do...@apache.org on 2026/10/05 21:01:34 UTC
(spark-kubernetes-operator) branch main updated: [SPARK-59984] Parse Boolean config values like Apache Spark and use the default value for null
This is an automated email from the ASF dual-hosted git repository.
dongjoon-hyun pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/spark-kubernetes-operator.git
The following commit(s) were added to refs/heads/main by this push:
new 833f05fc [SPARK-59984] Parse Boolean config values like Apache Spark and use the default value for null
833f05fc is described below
commit 833f05fcc255b891fdbbf84b3508ffb75a767966
Author: Dongjoon Hyun <do...@apache.org>
AuthorDate: Mon Oct 5 14:01:26 2026 -0700
[SPARK-59984] Parse Boolean config values like Apache Spark and use the default value for null
### What changes were proposed in this pull request?
This PR aims to make `ConfigOption` handle invalid values consistently.
- Parse a Boolean value like Apache Spark, i.e. `true` or `false` in any case, ignoring surrounding whitespace.
- Use the default value for an invalid value, including one which resolves to `null` like `null`.
- Log each invalid value once as a warning.
### Why are the changes needed?
Since every option uses a boxed type like `Boolean.class`, Jackson parses every non-String value. So, `False` falls back to the default value, `0` and `1` are taken as `false` and `true`, and `null` resolves to `null`, which causes `NullPointerException`. In addition, an invalid value is logged as an error with a stack trace on every read.
### Does this PR introduce _any_ user-facing change?
Yes. A Boolean value like `False` or `TRUE` is respected now, while `0`, `1`, `yes`, `off` or `null` falls back to the default value. The migration guide is updated.
### How was this patch tested?
Pass the CIs with the newly added test cases.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 5.5
Closes #935 from dongjoon-hyun/SPARK-59984.
Authored-by: Dongjoon Hyun <do...@apache.org>
Signed-off-by: Dongjoon Hyun <do...@apache.org>
---
docs/configuration.md | 4 +
docs/migration_guide.md | 8 ++
.../spark/k8s/operator/config/ConfigOption.java | 60 +++++++++---
.../operator/utils/ConfigurableEventRecorder.java | 9 +-
.../k8s/operator/config/ConfigOptionTest.java | 108 +++++++++++++++++++++
.../utils/ConfigurableEventRecorderTest.java | 14 +--
6 files changed, 174 insertions(+), 29 deletions(-)
diff --git a/docs/configuration.md b/docs/configuration.md
index 56bba661..d6a6dd2c 100644
--- a/docs/configuration.md
+++ b/docs/configuration.md
@@ -40,6 +40,10 @@ Spark Operator supports different ways to configure the behavior:
* Please be advised that not all properties can be hot-loaded and honored at runtime.
Refer the list of [supported properties](./config_properties.md) for more details.
+A Boolean property accepts `true` or `false` in any case, ignoring surrounding whitespace. If a
+property has an invalid value, e.g. `yes` for a Boolean property or `null`, operator ignores it and
+uses the default value instead, logging a warning once for each invalid value.
+
To enable hot properties loading, update the **helm chart values file** with
```yaml
diff --git a/docs/migration_guide.md b/docs/migration_guide.md
index 7370961a..a1852739 100644
--- a/docs/migration_guide.md
+++ b/docs/migration_guide.md
@@ -108,6 +108,14 @@ affect building the project, CI, tests or examples.
`0.5` and `0.999` quantiles of the `operator_sdk_*` histograms are the median and the 99.9th
percentile instead of the mean and the 99th percentile. A histogram or timer of a custom metrics
source has no `_sum` unless it is a `SummingHistogram` or a `SummingTimer` ([SPARK-59935](https://issues.apache.org/jira/browse/SPARK-59935)).
+- Since 1.1.0, a Boolean configuration property accepts `true` or `false` in any case, ignoring
+ surrounding whitespace, like Apache Spark. Any other value, e.g. `0`, `1`, `yes` or `off`, is
+ ignored, and the default value is used. 1.0 ignored a value like `False` or `TRUE` and used the
+ default value, while it took `0` as `false` and any other integer as `true`. Likewise, `null`
+ falls back to the default value for a property of any type, while 1.0 failed with a
+ `NullPointerException` for most properties. The operator logs each invalid value once as a
+ warning, instead of as an error with a stack trace whenever it reads the value. Use `true` or
+ `false`, which behave the same in both versions ([SPARK-XXXXX](https://issues.apache.org/jira/browse/SPARK-XXXXX)).
### SparkApplication
diff --git a/spark-operator/src/main/java/org/apache/spark/k8s/operator/config/ConfigOption.java b/spark-operator/src/main/java/org/apache/spark/k8s/operator/config/ConfigOption.java
index d9c4aae9..797b698f 100644
--- a/spark-operator/src/main/java/org/apache/spark/k8s/operator/config/ConfigOption.java
+++ b/spark-operator/src/main/java/org/apache/spark/k8s/operator/config/ConfigOption.java
@@ -51,6 +51,9 @@ public class ConfigOption<T> {
/** Indexes every declared option so dynamic-config refresh can check overridability. */
private static final Map<String, ConfigOption<?>> REGISTRY = new ConcurrentHashMap<>();
+ /** Invalid values already logged, as {@code key=value}, so that each is logged only once. */
+ private static final Set<String> LOGGED_INVALID_VALUES = ConcurrentHashMap.newKeySet();
+
/**
* 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.
@@ -100,7 +103,9 @@ public class ConfigOption<T> {
}
/**
- * Returns the resolved value of the config option.
+ * Returns the resolved value of the config option. An unset or empty value resolves to the
+ * default value, and so does an invalid value, e.g. one which cannot be parsed or which resolves
+ * to null like {@code null}, for which a warning is logged once.
*
* @return The resolved value.
*/
@@ -113,40 +118,65 @@ public class ConfigOption<T> {
}
private T resolveValue() {
+ String value = SparkOperatorConfManager.INSTANCE.getValue(key);
+ if (!enableDynamicOverride) {
+ value = SparkOperatorConfManager.INSTANCE.getInitialValue(key);
+ }
try {
- String value = SparkOperatorConfManager.INSTANCE.getValue(key);
- if (!enableDynamicOverride) {
- value = SparkOperatorConfManager.INSTANCE.getInitialValue(key);
- }
if (StringUtils.isNotEmpty(value)) {
- if (typeParameterClass.isPrimitive() || typeParameterClass == String.class) {
+ if (typeParameterClass.isPrimitive()
+ || typeParameterClass == String.class
+ || typeParameterClass == Boolean.class) {
return (T) resolveValueToPrimitiveType(typeParameterClass, value);
} else {
- return ModelUtils.objectMapper.readValue(value, typeParameterClass);
+ T resolvedValue = ModelUtils.objectMapper.readValue(value, typeParameterClass);
+ // A JSON null like 'null' is not a valid value either.
+ return resolvedValue == null ? defaultValueInsteadOf(value) : resolvedValue;
}
} else {
return defaultValue;
}
- } catch (NumberFormatException | JsonProcessingException t) {
- log.error(
- "Failed to resolve value for config key {}, using default value {}",
+ } catch (IllegalArgumentException | JsonProcessingException e) {
+ return defaultValueInsteadOf(value);
+ }
+ }
+
+ /**
+ * Returns the default value in place of the given invalid value. The warning is logged only the
+ * first time the invalid value is seen for this key, since an option may be read on every
+ * reconciliation, or even on every request to the API server.
+ *
+ * @param invalidValue The invalid value.
+ * @return The default value.
+ */
+ private T defaultValueInsteadOf(String invalidValue) {
+ if (LOGGED_INVALID_VALUES.add(key + "=" + invalidValue)) {
+ log.warn(
+ "Invalid {} value '{}' for config key {}, using default value {}",
+ typeParameterClass.getSimpleName(),
+ invalidValue,
key,
- defaultValue,
- t);
- return defaultValue;
+ defaultValue);
}
+ return defaultValue;
}
/**
- * Resolves a string value to a primitive type or String.
+ * Resolves a string value to a primitive type or String. Like Apache Spark, a boolean value must
+ * be 'true' or 'false' in any case, ignoring surrounding whitespace.
*
* @param clazz The class of the target type.
* @param value The string value to resolve.
* @return The resolved value as an Object.
+ * @throws IllegalArgumentException If the value is invalid for the target type.
*/
public static Object resolveValueToPrimitiveType(Class<?> clazz, String value) {
if (Boolean.class == clazz || Boolean.TYPE == clazz) {
- return Boolean.parseBoolean(value);
+ String trimmed = value.trim();
+ if ("true".equalsIgnoreCase(trimmed) || "false".equalsIgnoreCase(trimmed)) {
+ return Boolean.parseBoolean(trimmed);
+ }
+ throw new IllegalArgumentException("Invalid boolean value: " + value);
}
if (Byte.class == clazz || Byte.TYPE == clazz) {
return Byte.parseByte(value);
diff --git a/spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ConfigurableEventRecorder.java b/spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ConfigurableEventRecorder.java
index 7baff8a7..5170a53b 100644
--- a/spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ConfigurableEventRecorder.java
+++ b/spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ConfigurableEventRecorder.java
@@ -159,9 +159,7 @@ public class ConfigurableEventRecorder implements EventRecorder {
}
private static boolean eventsEnabled() {
- // Boolean.TRUE.equals guards against a null resolved value, which the option can yield for a
- // malformed override. Publishing events must never break a reconciliation.
- return Boolean.TRUE.equals(KUBERNETES_EVENTS_ENABLED.getValue());
+ return KUBERNETES_EVENTS_ENABLED.getValue();
}
private static boolean isExcluded(String reason) {
@@ -271,10 +269,7 @@ public class ConfigurableEventRecorder implements EventRecorder {
}
private static long minIntervalNanos() {
- Long seconds = KUBERNETES_EVENTS_MIN_INTERVAL_SECONDS.getValue();
- // An unparseable override falls back to the default, but an override of the literal 'null'
- // resolves to null, which is treated as no limit rather than throwing out of a reconciliation.
- return seconds == null ? 0L : TimeUnit.SECONDS.toNanos(seconds);
+ return TimeUnit.SECONDS.toNanos(KUBERNETES_EVENTS_MIN_INTERVAL_SECONDS.getValue());
}
private static boolean matches(String regex, String reason) {
diff --git a/spark-operator/src/test/java/org/apache/spark/k8s/operator/config/ConfigOptionTest.java b/spark-operator/src/test/java/org/apache/spark/k8s/operator/config/ConfigOptionTest.java
index c380cc33..03bf9369 100644
--- a/spark-operator/src/test/java/org/apache/spark/k8s/operator/config/ConfigOptionTest.java
+++ b/spark-operator/src/test/java/org/apache/spark/k8s/operator/config/ConfigOptionTest.java
@@ -20,12 +20,27 @@
package org.apache.spark.k8s.operator.config;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
+import java.util.concurrent.CopyOnWriteArrayList;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.LoggerContext;
+import org.apache.logging.log4j.core.appender.AbstractAppender;
+import org.apache.logging.log4j.core.config.Property;
+import org.apache.logging.log4j.core.layout.PatternLayout;
+import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
class ConfigOptionTest {
+ @AfterEach
+ void resetConf() {
+ SparkOperatorConfManager.INSTANCE.refresh(Map.of());
+ }
+
@Test
void testResolveValueWithoutOverride() {
byte defaultByteValue = 9;
@@ -205,4 +220,97 @@ class ConfigOptionTest {
Assertions.assertEquals(overrideShortValue, testShortConf.getValue());
Assertions.assertEquals(overrideDoubleValue, testDoubleConf.getValue());
}
+
+ @Test
+ void testResolveBooleanValueIgnoringCaseAndSurroundingWhitespace() {
+ ConfigOption<Boolean> falseByDefault = dynamicOption("fooboolcasetrue", Boolean.class, false);
+ for (String value : List.of("true", "TRUE", "True", " true ", "true ")) {
+ SparkOperatorConfManager.INSTANCE.refresh(Map.of(falseByDefault.getKey(), value));
+ Assertions.assertTrue(falseByDefault.getValue(), value);
+ }
+ ConfigOption<Boolean> trueByDefault = dynamicOption("fooboolcasefalse", Boolean.class, true);
+ for (String value : List.of("false", "FALSE", "False", " false ", "false ")) {
+ SparkOperatorConfManager.INSTANCE.refresh(Map.of(trueByDefault.getKey(), value));
+ Assertions.assertFalse(trueByDefault.getValue(), value);
+ }
+ }
+
+ @Test
+ void testResolveInvalidValueToDefaultValue() {
+ // Unlike Boolean.parseBoolean, an invalid value does not turn off an option enabled by default.
+ ConfigOption<Boolean> trueByDefault = dynamicOption("fooboolinvalidtrue", Boolean.class, true);
+ for (String value : List.of("0", "off", "no", "f", "\"false\"", "null")) {
+ SparkOperatorConfManager.INSTANCE.refresh(Map.of(trueByDefault.getKey(), value));
+ Assertions.assertTrue(trueByDefault.getValue(), value);
+ }
+ ConfigOption<Boolean> falseByDefault =
+ dynamicOption("fooboolinvalidfalse", Boolean.class, false);
+ for (String value : List.of("1", "-1", "on", "yes", "t", "\"true\"", "true,false")) {
+ SparkOperatorConfManager.INSTANCE.refresh(Map.of(falseByDefault.getKey(), value));
+ Assertions.assertFalse(falseByDefault.getValue(), value);
+ }
+ // A value which resolves to null is invalid for any type.
+ ConfigOption<Long> longConf = dynamicOption("foolonginvalid", Long.class, 9L);
+ for (String value : List.of("null", "\"\"", "\"null\"", "abc")) {
+ SparkOperatorConfManager.INSTANCE.refresh(Map.of(longConf.getKey(), value));
+ Assertions.assertEquals(9L, longConf.getValue(), value);
+ }
+ }
+
+ @Test
+ void testLogEachInvalidValueOnce() {
+ ConfigOption<Boolean> testBooleanConf = dynamicOption("fooboolwarn", Boolean.class, true);
+ TestLogAppender appender = new TestLogAppender();
+ appender.start();
+ LoggerContext ctx = (LoggerContext) LogManager.getContext(false);
+ ctx.getConfiguration().getRootLogger().addAppender(appender, Level.WARN, null);
+ ctx.updateLoggers();
+ try {
+ for (String value : List.of("yes", "no")) {
+ SparkOperatorConfManager.INSTANCE.refresh(Map.of(testBooleanConf.getKey(), value));
+ Assertions.assertTrue(testBooleanConf.getValue());
+ Assertions.assertTrue(testBooleanConf.getValue());
+ }
+ Assertions.assertEquals(
+ List.of(
+ "Invalid Boolean value 'yes' for config key fooboolwarn, using default value true",
+ "Invalid Boolean value 'no' for config key fooboolwarn, using default value true"),
+ appender.messages);
+ } finally {
+ ctx.getConfiguration().getRootLogger().removeAppender(appender.getName());
+ ctx.updateLoggers();
+ appender.stop();
+ }
+ }
+
+ private static <T> ConfigOption<T> dynamicOption(String key, Class<T> clazz, T defaultValue) {
+ return ConfigOption.<T>builder()
+ .key(key)
+ .enableDynamicOverride(true)
+ .typeParameterClass(clazz)
+ .description("foo foo.")
+ .defaultValue(defaultValue)
+ .build();
+ }
+
+ /** Collects the messages logged by {@link ConfigOption}. */
+ private static final class TestLogAppender extends AbstractAppender {
+ private final List<String> messages = new CopyOnWriteArrayList<>();
+
+ private TestLogAppender() {
+ super(
+ "ConfigOptionTestLogAppender",
+ null,
+ PatternLayout.createDefaultLayout(),
+ false,
+ Property.EMPTY_ARRAY);
+ }
+
+ @Override
+ public void append(LogEvent event) {
+ if (ConfigOption.class.getName().equals(event.getLoggerName())) {
+ messages.add(event.getMessage().getFormattedMessage());
+ }
+ }
+ }
}
diff --git a/spark-operator/src/test/java/org/apache/spark/k8s/operator/utils/ConfigurableEventRecorderTest.java b/spark-operator/src/test/java/org/apache/spark/k8s/operator/utils/ConfigurableEventRecorderTest.java
index 195eae7a..445d556a 100644
--- a/spark-operator/src/test/java/org/apache/spark/k8s/operator/utils/ConfigurableEventRecorderTest.java
+++ b/spark-operator/src/test/java/org/apache/spark/k8s/operator/utils/ConfigurableEventRecorderTest.java
@@ -145,7 +145,7 @@ class ConfigurableEventRecorderTest {
@Test
void treatsAMalformedOverrideAsDisabled() {
- // The option is a boxed Boolean, so a malformed override can resolve to null. Publishing an
+ // A malformed override falls back to the default value, which is disabled. Publishing an
// event must never be the thing that throws out of a reconciliation.
SparkOperatorConfManager.INSTANCE.refresh(
Map.of(KUBERNETES_EVENTS_ENABLED.getKey(), "null"));
@@ -464,9 +464,9 @@ class ConfigurableEventRecorderTest {
}
@Test
- void treatsAMalformedMinIntervalOverrideAsNoLimit() {
- // The option is a boxed Long, so a malformed override can resolve to null. Unlike the enabled
- // flag, which then drops every event, a limit that cannot be read must not drop any.
+ void usesTheDefaultMinIntervalForAMalformedOverride() {
+ // A malformed override falls back to the default value like the enabled flag, rather than
+ // lifting the limit.
SparkOperatorConfManager.INSTANCE.refresh(
Map.of(
KUBERNETES_EVENTS_ENABLED.getKey(),
@@ -475,13 +475,13 @@ class ConfigurableEventRecorderTest {
"null"));
Context<?> context = contextOf("uid-1");
EventRecord first = EventRecord.normal("KueueAdmissionPending", "queued");
- EventRecord second = EventRecord.normal("KueueAdmissionPending", "queued");
timedRecorder.record(first, context);
- timedRecorder.record(second, context);
+ elapseSeconds(KUBERNETES_EVENTS_MIN_INTERVAL_SECONDS.getDefaultValue() - 1);
+ timedRecorder.record(EventRecord.normal("KueueAdmissionPending", "queued"), context);
verify(delegate).record(first, context);
- verify(delegate).record(second, context);
+ verifyNoMoreInteractions(delegate);
}
@Test
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@spark.apache.org
For additional commands, e-mail: commits-help@spark.apache.org