KAFKA-3711: Ensure a RecordingMap is passed to configured instances

[jklukas]

See https://issues.apache.org/jira/browse/KAFKA-3711

I've tested locally that this change does indeed resolve the warning I mention in the ticket:

org.apache.kafka.clients.consumer.ConsumerConfig: The configuration metric.dropwizard.registry = kafka-metrics was supplied but isn't a known config.

where metric.dropwizard.registry is a configuration value defined in a custom MetricReporter (https://github.com/SimpleFinance/kafka-dropwizard-reporter).

With this change, the above warning no longer appears, as @ewencp predicted.

This contribution is my original work and I license the work to the project under the project's open source license.

[jklukas]

cc @ijuma @guozhangwang

[ijuma]

LGTM. I'll let @ewencp check as well since he suggested the change.

[ewencp]

This seems fine for the specific issue. A few things:

1. I wonder if we should just make this.originals a RecordingMap to get rid of this issue entirely -- any use of this.originals would have the right behavior.
2. Unit test?
3. I think originalsWithPrefix may not work correctly still. I think for that case, the implementation of RecordingMap needs to take the prefix and add it to the key when recording each used key.

3 could easily be left to a separate JIRA (Connect is the only user of this at the moment, although increasingly I think we should be adopting the approach to avoid a lot of this mess of shared namespaces and trying to track shared usage of configs).

[ijuma]

@ewencp, the issue with assigning RecordingMap to originals is that we may mark a config as used inadvertently (e.g. in the unused() method). But it looks like the distinction between originals and originals() is too subtle and error-prone. Perhaps, we should have a field or method with a different name instead of originals() (recordingOriginals?).

Good catch about originalsWithPrefix.

[ewencp]

@ijuma Ah, that makes sense. I'm actually ok with it as is, we just have to take care when adding anything to this class. It's relatively low churn these days, so it's not a huge concern to me. However, we definitely should have a unit test.

[jklukas]

I will work on adding a unit test. I'll also take a look at originalsWithPrefix.

[jklukas]

This should be ready for review again. Pushed two commits that add tests and allow RecordingMap to handle a prefix.

[ijuma]

We should probably avoid string concatenation if prefix.isEmpty (since it's the common case).

[jklukas]

We now check prefix.isEmpty() here to avoid concatentation in most cases and save a String allocation, as suggested by @ijuma

[ewencp]

LGTM

[guozhangwang]

LGTM!