C0urante opened a new pull request #11986: URL: https://github.com/apache/kafka/pull/11986
[Jira](https://issues.apache.org/jira/browse/KAFKA-7509) **NOTE: This may require a KIP. Please do not merge until it has been explicitly confirmed that a KIP is not required, or a KIP for these changes has been published and approved.** ### Summary of changes - Skip the calls to `AbstractConfig::logUnused` made by [KafkaConsumer](https://github.com/apache/kafka/blob/62ea4c46a9be7388baeaef1c505d3e5798a9066f/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L815), [KafkaProducer](https://github.com/apache/kafka/blob/62ea4c46a9be7388baeaef1c505d3e5798a9066f/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java#L432), and [KafkaAdminClient](https://github.com/apache/kafka/blob/62ea4c46a9be7388baeaef1c505d3e5798a9066f/clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java#L595) instances when the original config map is an instance of a [RecordingMap](https://github.com/apache/kafka/blob/62ea4c46a9be7388baeaef1c505d3e5798a9066f/clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java#L608-L612) - Modify `ConsumerConfig::appendDeserializerToConfig` and `ProducerConfig::appendSerializerToConfig` to preserve `RecordingMap` instances passed in to their constructors (or more precisely, create clones of those instances that retain the "recording" behavior of the original) so that all properties used by those consumers/producers are marked as used with the original `RecordingMap` - Use `WorkerConfig::originals` as the baseline when constructing configs to pass to Kafka clients that are used by the worker to manage its internal Kafka topics, so that all properties in the worker config that are used by those Kafka clients are marked as used in the `WorkerConfig` - Ignore all properties in the worker config that are transparently passed through to configurations for other components that: - - Perform their own logging for unused properties (such as producers and consumers used by connector instances, whose properties can be specified in a worker config with the `producer.` and `consumer.` prefixes, respectively) - - Are used transparently by the worker without accessing via either `AbstractConfig::get` (or one of its strongly-typed variants) or by invoking `Map::get` on the result of `AbstractConfig::originals` (or one of its prefixed variants) (such as internal topic settings) - - Are not constructed during worker startup, but instead brought up later (such as the default key, value, and header converters, which are instantiated on a case-by-case basis when bringing up connectors) - Log warnings for all unused (and non-ignored) properties in the `WorkerConfig` after worker startup has taken place - Disable all warnings for unused properties when constructing admin clients used by connectors as those include the top-level worker config, which is guaranteed to contain properties like `key.converter` that are not used by the admin client - Permit all warnings for unused properties when constructing producers and consumers used by connectors as those do not include the top-level worker config and unused properties should not be expected in these cases - Automatically ignore all automatically-injected metrics context properties that are added by the Connect framework when configuring Kafka clients since these are always provided (when Connect brings up Kafka clients) but are not always used I also fixed a bug introduced in https://github.com/apache/kafka/pull/8455 that causes a spurious warning to be logged when the worker config doesn't include a value for the `plugin.path` property. ### Testing I've verified this locally with a variety of cases including typos in the worker config (`gorup.id` instead of `group.id`), typos in connector client properties included in the worker config (`producer.clinet.id` instead of `producer.client.id`), correctly-skipped connector client properties included in the worker config (`consumer.max.poll.records`), connector client interceptor properties included in the worker config (`producer.interceptor.classes`, `some.interceptor.property.that.is.used`, `some.interceptor.property.that.is.not.used`), use of the DLQ topic in a sink connector, and use of automatic topic creation in a source connector. If this approach looks reasonable, I can automate these tests, probably by capturing logging output during an integration test run and asserting that warnings were issued only for the expected set of properties. ### Edge cases Note that the `RecordingMap` class is subtly broken at the moment in that it doesn't take into account calls to `Map::forEach`, `Map::entrySet`, `Map::keySet`, `Map::values`, `Map::getOrDefault`, `Map::compute`, `Map::computeIfPresent`, etc. This comes into play with cases like when custom settings are specified for internal topics (see [TopicAdmin.NewTopicBuilder::config](https://github.com/apache/kafka/blob/62ea4c46a9be7388baeaef1c505d3e5798a9066f/connect/runtime/src/main/java/org/apache/kafka/connect/util/TopicAdmin.java#L224-L240)). We may not want to invest too heavily into this approach for controlling warning messages if we want to develop a truly flexible solution that can be easily used by both internal and external components. ### Committer Checklist (excluded from commit message) - [ ] Verify design and implementation - [ ] Verify test coverage and CI build status - [ ] Verify documentation (including upgrade notes) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
