kafka-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From rsiva...@apache.org
Subject [kafka] branch trunk updated: KAFKA-8425: Fix for correctly handling immutable maps (KIP-421 bug) (#6795)
Date Mon, 03 Jun 2019 11:43:29 GMT
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 <43974298+tadsul@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 <rhauch@gmail.com>, Boyang Chen <bchen11@outlook.com>,
Rajini Sivaram <rajinisivaram@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 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>


Mime
View raw message