KAFKA-8425: Fix for correctly handling immutable maps (KIP-421 bug) by tadsul · Pull Request #6795 · apache/kafka

tadsul · 2019-05-23T15:15:26Z

The originals map passed to the AbstractConfig class can be immutable. The resolveConfigVariable function was modifying the originals map. If this map is immutable it will result in an exception.

This fix resolves this issue

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…. The resolveConfigVariable function was modifying the originals map. If this map is immutable it will result in an exception. This fix resolves this issue

abbccdda · 2019-05-23T17:18:49Z

    private  Map<String, ?> resolveConfigVariables(Map<String, ?> configProviderProps, Map<String, Object> originals) {
        Map<String, String> providerConfigString;
        Map<String, ?> configProperties;
+        Map<String, Object> originalsMutable = new HashMap<>();


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?

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

I'd suggest naming this variable resolvedOriginals.

rajinisivaram

@tadsul Thanks for the PR. Left a couple of comments. Can we make sure that we have a test that fails without the change?

rajinisivaram · 2019-05-23T17:26:57Z

    private  Map<String, ?> resolveConfigVariables(Map<String, ?> configProviderProps, Map<String, Object> originals) {
        Map<String, String> providerConfigString;
        Map<String, ?> configProperties;
+        Map<String, Object> originalsMutable = new HashMap<>();


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

rajinisivaram · 2019-05-23T17:28:58Z

    }

+    @Test
+    public void testImmutableOriginalsWithConfigProvidersProps() {


Does this test fail without the change?

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

yes the test fails without the comment

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.

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

rajinisivaram · 2019-05-23T17:30:58Z

@tadsul There are checkstyle failures in the PR build. Could you please fix those as well?

rajinisivaram · 2019-05-23T17:37:37Z

@tadsul Can we create a JIRA and set 2.3.0 as fix version to make sure it gets merged into 2.3.0 before thre release?

rhauch · 2019-05-28T22:43:08Z

    private  Map<String, ?> resolveConfigVariables(Map<String, ?> configProviderProps, Map<String, Object> originals) {
        Map<String, String> providerConfigString;
        Map<String, ?> configProperties;
+        Map<String, Object> originalsMutable = new HashMap<>();


I'd suggest naming this variable resolvedOriginals.

rhauch · 2019-05-28T22:45:46Z

    }

+    @Test
+    public void testImmutableOriginalsWithConfigProvidersProps() {


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

rajinisivaram · 2019-05-29T11:24:31Z

@tadsul At the moment in AbstractConfig constructor, we are doing:

this.originals = resolveConfigVariables(configProviderProps, (Map<String, Object>) originals);

This doesn't work as expected for two reasons:

If we resolve configs in originals and then invoke plugins with the originals that are already resolved, we resolve configs again. I think we dont want that behaviour. This was the case even before this PR.
As you already figured out, this PR breaks AbstractConfigTest#testUnused because we are replacing RecordingMap with a new map. We shouldn't do that either.

Can't think of a way to fix this behaviour neatly. The only hacky solution I can think of that fixes 1) and 2) would be to check if originals passed through the constructor is a RecordingMap and only resolve if it is not.

is already tested by AbstractConfigTest#testUnused. We should also add a test for 1).

2. Makes sure RecordMap instance is not overriden

…ap to track all the key access

tadsul · 2019-05-31T22:22:39Z

The 'originals' map in AbstractConfig can be mutable or immutable instance. In order to resolve and save the variable configs we have added a new ResolvingMap class. The ResolvingMap class extends from HashMap and keeps a track of original map instance and the resolved configs. It stores the original input map as-is in a separate internal immutable Map field, and stores all the entries (including the resolved ones) in the super.

The 'originals' map can also be an instance of RecordingMap. In order to ensure that all the usage are recorded correctly the ResolvingMap overrides get() function to perform the following

always call originals.get(key) to record the access.
always return super.get(key) so it it returns the resolved value to the caller.

rhauch · 2019-05-31T22:32:56Z

This change will require #6855 to be merged first, which addresses an issue with this PR's builds in which the Kafka Streams tests changed the originals map after instantiating the StreamsConfig class. Once that is merged, this PR's builds should pass.

rhauch

Thanks, @tadsul! LGTM, pending a green build after #6855 is merged.

rhauch · 2019-05-31T22:36:15Z

This will need to be backported to the 2.3 branch, which is the first branch with the config variable resolution via config providers.

guozhangwang · 2019-06-01T00:03:06Z

#6855 has been merged. Retest this please

rhauch · 2019-06-02T14:18:29Z

retest this please

…actConfig$ResolvingMap

rajinisivaram

@tadsul Thanks for the updates, LGTM. Merging to trunk and 2.3.

…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>

* AK_REPO/2.3: KAFKA-8155: Add 2.2.0 release to system tests (apache#6597) KAFKA-8404: Add HttpHeader to RestClient HTTP Request and Connector REST API (apache#6791) KAFKA-8473: Adjust Connect system tests for incremental cooperative rebalancing (apache#6872) KAFKA-8475: Temporarily restore SslFactory.sslContext() helper KAFKA-8449: Restart tasks on reconfiguration under incremental cooperative rebalancing (apache#6850) KAFKA-8426; Fix for keeping the ConfigProvider configs consistent with KIP-297 (apache#6750) KAFKA-8425: Fix for correctly handling immutable maps (KIP-421 bug) (apache#6795) MINOR: Reordering the props modification with configs construction

…pache#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>

The originals map passed to the AbstractConfig class can be immutable…

94f0767

…. The resolveConfigVariable function was modifying the originals map. If this map is immutable it will result in an exception. This fix resolves this issue

abbccdda reviewed May 23, 2019

View reviewed changes

rajinisivaram reviewed May 23, 2019

View reviewed changes

rhauch reviewed May 28, 2019

View reviewed changes

rajinisivaram changed the title ~~KIP_421 bug: Fix for correctly handling immutable maps~~ KAFKA-8425; KIP_421 bug: Fix for correctly handling immutable maps May 29, 2019

tadsul added 2 commits May 29, 2019 04:53

Code Review Fixes - 1. Makes sure the originals map is immutable

1ec3053

2. Makes sure RecordMap instance is not overriden

Code Review Fix to handle RecordingMap by adding a new map ResolvingM…

f89a914

…ap to track all the key access

tadsul force-pushed the immutableMapFix branch 2 times, most recently from 4fc4c50 to f89a914 Compare May 31, 2019 21:39

rhauch approved these changes May 31, 2019

View reviewed changes

KAFKA-8425: Modified the JavaDoc and addressed code warnings in Abstr…

5d4bca6

…actConfig$ResolvingMap

rhauch changed the title ~~KAFKA-8425; KIP_421 bug: Fix for correctly handling immutable maps~~ KAFKA-8425: Fix for correctly handling immutable maps (KIP-421 bug) Jun 2, 2019

rajinisivaram approved these changes Jun 3, 2019

View reviewed changes

rajinisivaram merged commit 2c810e4 into apache:trunk Jun 3, 2019

Uh oh!

Conversation

tadsul commented May 23, 2019

Committer Checklist (excluded from commit message)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajinisivaram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajinisivaram commented May 23, 2019

Uh oh!

rajinisivaram commented May 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajinisivaram commented May 29, 2019

Uh oh!

tadsul commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhauch commented May 31, 2019

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

rhauch commented May 31, 2019

Uh oh!

guozhangwang commented Jun 1, 2019

Uh oh!

rhauch commented Jun 2, 2019

Uh oh!

rajinisivaram left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tadsul commented May 31, 2019 •

edited

Loading