Skip to content

KAFKA-8425: Fix for correctly handling immutable maps (KIP-421 bug) #6795

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rajinisivaram merged 4 commits into from
Jun 3, 2019
Merged

KAFKA-8425: Fix for correctly handling immutable maps (KIP-421 bug) #6795

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
94f0767
The originals map passed to the AbstractConfig class can be immutable…
tadsul May 23, 2019
1ec3053
Code Review Fixes - 1. Makes sure the originals map is immutable
tadsul May 29, 2019
f89a914
Code Review Fix to handle RecordingMap by adding a new map ResolvingM…
tadsul May 29, 2019
5d4bca6
KAFKA-8425: Modified the JavaDoc and addressed code warnings in Abstr…
rhauch Jun 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,9 @@ private Map<String, String> extractPotentialVariables(Map<?, ?> configMap) {
private Map<String, ?> resolveConfigVariables(Map<String, ?> configProviderProps, Map<String, Object> originals) {
Map<String, String> providerConfigString;
Map<String, ?> configProperties;
Map<String, Object> originalsMutable = new HashMap<>();

@abbccdda abbccdda May 23, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit weird to convert an immutable map to mutable and never convert back. Could we refactor the logic to push up the modification logic?

@rajinisivaram rajinisivaram May 23, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tadsul We could perhaps convert this to a immutable map and store in originalslike we do forvalues`.

@rhauch rhauch May 28, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest naming this variable resolvedOriginals.


originalsMutable.putAll(originals);

// As variable configs are strings, parse the originals and obtain the potential variable configs.
Map<String, String> indirectVariables = extractPotentialVariables(originals);
Expand All @@ -475,10 +478,10 @@ private Map<String, String> extractPotentialVariables(Map<?, ?> configMap) {
if (!providers.isEmpty()) {
ConfigTransformer configTransformer = new ConfigTransformer(providers);
ConfigTransformerResult result = configTransformer.transform(indirectVariables);
originals.putAll(result.data());
originalsMutable.putAll(result.data());
}

return originals;
return originalsMutable;
Comment thread
tadsul marked this conversation as resolved.
Outdated
}

private Map<String, Object> configProviderProperties(String configProviderPrefix, Map<String, ?> providerConfigProperties) {
Expand Down
13 changes: 13 additions & 0 deletions clients/src/test/java/org/apache/kafka/common/config/AbstractConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,19 @@ public void testConfigProvidersPropsAsParam() {
assertEquals(config.originals().get("sasl.kerberos.password"), "randomPassword");
}

@Test
public void testImmutableOriginalsWithConfigProvidersProps() {

@rajinisivaram rajinisivaram May 23, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this test fail without the change?

@rhauch rhauch May 28, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tadsul, what is the answer to this question? (Unresolving this conversation.)

@tadsul tadsul May 28, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes the test fails without the comment

@rajinisivaram rajinisivaram May 29, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I am not sure it would. You should run the test without the changes to AbstractConfig.java to verify. The test invokes new TestIndirectConfigResolution(props, immutableMap) where neither props nor immutableMap is actually immutable. In order fot the test to fail props needs to be unmodifiable since that is the map used as originals. And immutableMap needs to be renamed since it is confusing as it is neither immutable nor the map being tested for immutability.

@tadsul tadsul May 29, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yes i had to modify the test to make it work .. my yes meant that i could reproduce the issue.. sorry about that i will post the modified test

// 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}");
final Map<String, ?> immutableMap = convertPropertiesToMap(providers);
TestIndirectConfigResolution config = new TestIndirectConfigResolution(props, immutableMap);
assertEquals(config.originals().get("sasl.kerberos.key"), "testKey");
}

@Test
public void testAutoConfigResolutionWithMultipleConfigProviders() {
// Test Case: Valid Test Case With Multiple ConfigProviders as a separate variable
Expand Down