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-7449 Forward topic from console consumer to deserializer #5704

Merged
merged 1 commit into from
Nov 29, 2018

Conversation

mchataigner
Copy link
Contributor

Some deserializer needs the topic name to be able to correctly deserialize the payload of the message.
Console consumer works great with Deserializer however it calls deserializer with topic set as null.
This breaks the API and the topic information is available in the ConsumerRecord.

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

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

@omkreddy
Copy link
Contributor

LGTM

@chia7712
Copy link
Contributor

LGTM. Does it need a test? ConsoleConsumerTest should be a suitable place.

@ijuma
Copy link
Contributor

ijuma commented Sep 29, 2018

Yes, a test would be good.

@mchataigner mchataigner force-pushed the deserializer branch 2 times, most recently from cb6e3d3 to db6be73 Compare October 24, 2018 16:48
@mchataigner
Copy link
Contributor Author

Done (sorry for the delay).

@mchataigner mchataigner force-pushed the deserializer branch 4 times, most recently from 400608b to 6b0b521 Compare October 29, 2018 09:48
@mchataigner
Copy link
Contributor Author

I don't realy understand why the build on JDK 11 with Scala 2.12 is ending in timeout.
I'm not sure it's related to my change.

Some deserializer needs the topic name to be able to correctly deserialize the payload of the message.
Console consumer works great with Deserializer<String> however it calls deserializer with topic set as null.
This breaks the API and the topic information is available in the ConsumerRecord.
@gardnervickers
Copy link
Contributor

Hmm not sure why the test is being skipped for the JDK 8 build, lets have Jenkins rerun the tests.
Retest this please

Copy link
Contributor

@junrao junrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mchataigner : Thanks for the patch. LGTM

@junrao junrao merged commit fb9f2d8 into apache:trunk Nov 29, 2018
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
Some deserializer needs the topic name to be able to correctly deserialize the payload of the message.
Console consumer works great with Deserializer<String> however it calls deserializer with topic set as null.
This breaks the API and the topic information is available in the ConsumerRecord.

Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>, Gardner Vickers <gardner@vickers.me>, Jun Rao <junrao@gmail.com>
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.

6 participants