This is an automated email from the ASF dual-hosted git repository.
rsivaram pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/kafka.git
The following commit(s) were added to refs/heads/trunk by this push:
new 2c810e4 KAFKA-8425: Fix for correctly handling immutable maps
(KIP-421 bug) (#6795)
2c810e4 is described below
commit 2c810e4afb1b41ec1f8565d9a830d479b29dc708
Author: tadsul <[email protected]>
AuthorDate: Mon Jun 3 04:43:11 2019 -0700
KAFKA-8425: Fix for correctly handling immutable maps (KIP-421 bug) (#6795)
Since the originals map passed to AbstractConfig constructor may be
immutable, avoid updating this map while resolving indirect config variables.
Instead a new ResolvingMap instance is now used to store resolved configs.
Reviewers: Randall Hauch <[email protected]>, Boyang Chen
<[email protected]>, Rajini Sivaram <[email protected]>
---
.../apache/kafka/common/config/AbstractConfig.java | 37 +++++++++++++++++++---
.../kafka/common/config/AbstractConfigTest.java | 14 ++++++++
gradle/spotbugs-exclude.xml | 7 ++++
3 files changed, 54 insertions(+), 4 deletions(-)
diff --git
a/clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java
b/clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java
index 555b634..13865d0 100644
--- a/clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java
+++ b/clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java
@@ -103,7 +103,6 @@ public class AbstractConfig {
throw new ConfigException(entry.getKey().toString(),
entry.getValue(), "Key must be a string.");
this.originals = resolveConfigVariables(configProviderProps,
(Map<String, Object>) originals);
-
this.values = definition.parse(this.originals);
Map<String, Object> configUpdates =
postProcessParsedConfig(Collections.unmodifiableMap(this.values));
for (Map.Entry<String, Object> update : configUpdates.entrySet()) {
@@ -459,10 +458,11 @@ public class AbstractConfig {
private Map<String, ?> resolveConfigVariables(Map<String, ?>
configProviderProps, Map<String, Object> originals) {
Map<String, String> providerConfigString;
Map<String, ?> configProperties;
-
+ Map<String, Object> resolvedOriginals = new HashMap<>();
// As variable configs are strings, parse the originals and obtain the
potential variable configs.
Map<String, String> indirectVariables =
extractPotentialVariables(originals);
+ resolvedOriginals.putAll(originals);
if (configProviderProps == null || configProviderProps.isEmpty()) {
providerConfigString = indirectVariables;
configProperties = originals;
@@ -475,10 +475,12 @@ public class AbstractConfig {
if (!providers.isEmpty()) {
ConfigTransformer configTransformer = new
ConfigTransformer(providers);
ConfigTransformerResult result =
configTransformer.transform(indirectVariables);
- originals.putAll(result.data());
+ if (!result.data().isEmpty()) {
+ resolvedOriginals.putAll(result.data());
+ }
}
- return originals;
+ return new ResolvingMap<>(resolvedOriginals, originals);
}
private Map<String, Object> configProviderProperties(String
configProviderPrefix, Map<String, ?> providerConfigProperties) {
@@ -585,4 +587,31 @@ public class AbstractConfig {
return super.get(key);
}
}
+
+ /**
+ * ResolvingMap keeps a track of the original map instance and the
resolved configs.
+ * The originals are tracked in a separate nested map and may be a
`RecordingMap`; thus
+ * any access to a value for a key needs to be recorded on the originals
map.
+ * The resolved configs are kept in the inherited map and are therefore
mutable, though any
+ * mutations are not applied to the originals.
+ */
+ private static class ResolvingMap<V> extends HashMap<String, V> {
+
+ private final Map<String, ?> originals;
+
+ ResolvingMap(Map<String, ? extends V> resolved, Map<String, ?>
originals) {
+ super(resolved);
+ this.originals = Collections.unmodifiableMap(originals);
+ }
+
+ @Override
+ public V get(Object key) {
+ if (key instanceof String && originals.containsKey(key)) {
+ // Intentionally ignore the result; call just to mark the
original entry as used
+ originals.get(key);
+ }
+ // But always use the resolved entry
+ return super.get(key);
+ }
+ }
}
diff --git
a/clients/src/test/java/org/apache/kafka/common/config/AbstractConfigTest.java
b/clients/src/test/java/org/apache/kafka/common/config/AbstractConfigTest.java
index a007fd3..33aaac5 100644
---
a/clients/src/test/java/org/apache/kafka/common/config/AbstractConfigTest.java
+++
b/clients/src/test/java/org/apache/kafka/common/config/AbstractConfigTest.java
@@ -364,6 +364,20 @@ public class AbstractConfigTest {
}
@Test
+ public void testImmutableOriginalsWithConfigProvidersProps() {
+ // Test Case: Valid Test Case for ConfigProviders as a separate
variable
+ Properties providers = new Properties();
+ providers.put("config.providers", "file");
+ providers.put("config.providers.file.class",
"org.apache.kafka.common.config.provider.MockFileConfigProvider");
+ Properties props = new Properties();
+ props.put("sasl.kerberos.key", "${file:/usr/kerberos:key}");
+ Map<?, ?> immutableMap = Collections.unmodifiableMap(props);
+ Map<String, ?> provMap = convertPropertiesToMap(providers);
+ TestIndirectConfigResolution config = new
TestIndirectConfigResolution(immutableMap, provMap);
+ assertEquals(config.originals().get("sasl.kerberos.key"), "testKey");
+ }
+
+ @Test
public void testAutoConfigResolutionWithMultipleConfigProviders() {
// Test Case: Valid Test Case With Multiple ConfigProviders as a
separate variable
Properties providers = new Properties();
diff --git a/gradle/spotbugs-exclude.xml b/gradle/spotbugs-exclude.xml
index eeda703..6f91df0 100644
--- a/gradle/spotbugs-exclude.xml
+++ b/gradle/spotbugs-exclude.xml
@@ -334,4 +334,11 @@ For a detailed description of spotbugs bug categories, see
https://spotbugs.read
<!-- END Suppress warnings for unused members that are undetectably used
by Jackson -->
+ <Match>
+ <!-- Suppress a warning about ignoring return value because this is
intentional. -->
+ <Class
name="org.apache.kafka.common.config.AbstractConfig$ResolvingMap"/>
+ <Method name="get"/>
+ <Bug pattern="RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT"/>
+ </Match>
+
</FindBugsFilter>
