Skip to content

Simplify KafkaConsumer constructor logic#8707

Closed
nresare wants to merge 1 commit into
apache:trunkfrom
nresare:simplify_consumer_constructor_calls
Closed

Simplify KafkaConsumer constructor logic#8707
nresare wants to merge 1 commit into
apache:trunkfrom
nresare:simplify_consumer_constructor_calls

Conversation

@nresare

@nresare nresare commented May 21, 2020

Copy link
Copy Markdown
Contributor

This mirrors how the KafkaProducer constructors are organised and reduce
code duplication.

More specifically this PR makes the following changes without changing any behaviour or public API:

  1. Move the helper method propsToMap() from KafkaProducer to Utils with the other
    similar helpers mkMap() and mkProperties(), as this is now both used by KafkaConsumer and KafkaProducer.
  2. Move the ConsumerConfig instantiation to the constructor that actually does the work.
  3. Remove the addDeserializerToConfig() that operates on Properties and it's two tests, as all of those are very similar to the ones that operate on Map<String, Object> instances.
  4. Have the KafkaConsumer constructors that accept Properties parameter convert those early to Map<String, Object> instances.

Committer Checklist (excluded from commit message)

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

This mirrors how the KafkaProducer constructors are organised and reduce
code duplication.
@chia7712

Copy link
Copy Markdown
Member

It seems #8605 is duplicate to this issue.

@nresare

nresare commented May 21, 2020

Copy link
Copy Markdown
Contributor Author

@chia7712 what an interesting coincidence that our changes looks almost identical although I can assure you that they have been developed completely independently

@ijuma

ijuma commented May 30, 2020

Copy link
Copy Markdown
Member

Closing in favor of #8605.

@ijuma ijuma closed this May 30, 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

Development

Successfully merging this pull request may close these issues.

3 participants