You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kafka.apache.org by rs...@apache.org on 2019/06/03 11:51:17 UTC
[kafka] 02/02: KAFKA-8425: Fix for correctly handling immutable
maps (KIP-421 bug) (#6795)
This is an automated email from the ASF dual-hosted git repository.
rsivaram pushed a commit to branch 2.3
in repository https://gitbox.apache.org/repos/asf/kafka.git
commit dd763fbd6a9b1d712b1c3dbd1fbb497943cd28a6
Author: tadsul <43...@users.noreply.github.com>
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 <rh...@gmail.com>, Boyang Chen <bc...@outlook.com>, Rajini Sivaram <ra...@googlemail.com>
---
.../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 721b05e..60e2eb2 100644
--- a/gradle/spotbugs-exclude.xml
+++ b/gradle/spotbugs-exclude.xml
@@ -332,4 +332,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>