KAFKA-7509: Clean up incorrect warnings logged by Connect

[C0urante]

Jira

Summary of changes

• Skip the calls to AbstractConfig::logUnused made by KafkaConsumer, KafkaProducer, and KafkaAdminClient instances when the original config map is an instance of a RecordingMap
• 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 #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). 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)

[C0urante]

We could potentially replace this with actual instantiation and configuration of the key, value, and header converters specified in the worker config, but that may be wasteful of resources (especially since the Converter interface doesn't extend Closeable yet) and it's unclear how we'd want to handle failures encountered during that process (aborting worker startup is not an option as converter instantiation may fail due to transient errors).

[C0urante]

I've updated the approach to extract RecordingMap into a separate file that's in an internal package. It should now be acceptable to merge these changes without a KIP since that eliminates the change to the public Java API (i.e., exposing RecordingMap as a protected class of AbstractConfig).

[szalapski]

Reviewed and looks good, but I probably shouldn't be the one to approve.

I think the automated checks need to be addressed?

[szalapski]

Why not allow the concatenation when it is empty and thus get rid of the conditional?

[szalapski]

Great catch. Are there any other warns that should become infos elsewhere?

[C0urante]

Closing due to lack of review. We can revisit in the future, although the downgrade of these log messages from WARN to INFO level in #13225 likely addresses the underlying concern here that the WARN level is highly polluted with unused config property messages in Kafka Connect.