Skip to content
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.

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

KAFKA-10566: Fix erroneous config usage warnings #9365

Closed

Conversation

tombentley
Copy link
Contributor

Fix warn log messages caused by making HashMap copies of configs prior to using. This is not an ideal solution, but because the Maps are passed through Configurable.configure(Map) it's not possible to use another type (such as the RecordingMap type) directly.

@tombentley
Copy link
Contributor Author

@rajinisivaram @omkreddy perhaps one of you could review?

@tombentley
Copy link
Contributor Author

@rajinisivaram @omkreddy did you have any thoughts about this? What I've done here isn't exactly pretty, but it was the simplest thing I could think of which would remove the erroneous warnings.

As an aside: I spent quite some time looking at how the AbstractConfig tracks usage in order to log about unknown config. TBH it's a pattern which, as here, doesn't always work well, because what's passed to Configrable is a Map so it's easy for people to forget that they need to track usage if they need to configure a plugin with something other than AbstractConfig.values().

In some ways it would be better to be able to validate config parameters at the point of creating the AbstractConfig, but that would require lots of public API change to be able to build the overall ConfigDef (including all the plugins defined in the "root config"). As well as addressing the logging of unknown configs in a less flaky way, that would open the door for describeConfigs to be able to describe configs for plugins as well as the broker, for example. But it would be a lot of work if those were the only benefits. Thoughts?

@tombentley
Copy link
Contributor Author

@rajinisivaram @omkreddy @ijuma @mjsax please could someone take a look at this fix?

@tombentley
Copy link
Contributor Author

Maybe @chia7712, @dajac or @mimaison could take a look at this?

@chia7712
Copy link
Contributor

chia7712 commented Nov 4, 2020

Is it similar to https://issues.apache.org/jira/browse/KAFKA-10090?

@tombentley
Copy link
Contributor Author

@chia7712 I think there's overlap, but I think this one removes additional warnings because it also propagates usage tracking in SslFactory. (It also provides a non-public abstraction to as least make it easier to identify the places where we end up doing this usage tracking, which might be useful should we ever figure out a better way of doing this).

@tombentley tombentley closed this Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants