MINOR: align the constructor of KafkaConsumer to KafkaProducer

[chia7712]

This PR includes following changes.

1. remove KafkaProducer#propsToMap
2. align the constructor of KafkaProducer and KafkaConsumer

1. move KafkaProducer#propsToMap to Utils#propsToMap
2. apply Utils#propsToMap to constructor of KafkaConsumer

Committer Checklist (excluded from commit message)

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

[chia7712]

this constructor is used by KafkaProducerTest only. I keep this method to minimize this PR

[ijuma]

Thanks for the PR. Even though it removes some duplication, it adds some duplication that the previous approach avoided. To me, it's the approach proposed in this PR is more error prone.

[ijuma]

The previous approach was intended to avoid having this logic in many places.

[chia7712]

Thanks for reviews!

Do you mean that previous approach tried to avoid ‘’’addSerializerToConfig’’’? If so, should we do it for KafkaConsumer?

[ijuma]

Yeah, it's a bit error prone to have that logic in every constructor. We could move the propsToMap method to a utility class and use it on the consumer too.

[chia7712]

the access modifier is changed from private to public as

1. KafkaProducer has a same but public constructor
2. the previous constructor (ConsumerConfig, ...) is useless

[nresare]

@ijuma please note that this diff has changed fundamentally since you reviewed it, and it now modifies KafkaConsumer and not KafkaProducer.

I would like to endorse this PR. Funnily enough I came to very similar conclusions when I worked on the same code yesterday and raised #8707 without any knowledge that this one existed.

[nresare]

Please note that addDeserializerToConfig(Properties, Deserializer<?>, Deserializer<?>) is now only used in test cases and can be removed along with the two tests of that method.

[chia7712]

@nresare This "coincidence" is more favorite than "approved" :)

[nresare]

@chia7712 did you get around to update the PR with a variant that removes the no-longer-used addDeserializerToConfig() method?

[chia7712]

This test is similar to ConsumerConfigTest

[ijuma]

Thanks for the updates. A few comments below.

[ijuma]

Changes look good, just one comment below.

[ijuma]

Should we assert a string valued parameter too?

[chia7712]

done

[ijuma]

LGTM, thanks.