Skip to content

KAFKA-7678: Avoid NPE when closing the RecordCollector#5993

Merged
mjsax merged 2 commits intoapache:trunkfrom
jonathansantilli:KAFKA-7678
Dec 5, 2018
Merged

KAFKA-7678: Avoid NPE when closing the RecordCollector#5993
mjsax merged 2 commits intoapache:trunkfrom
jonathansantilli:KAFKA-7678

Conversation

@jonathansantilli
Copy link
Copy Markdown
Contributor

Check if the Producer is not null to avoid calling producer.close() on a null value.

@jonathansantilli jonathansantilli changed the title Avoid NPE when closing the RecordCollector KAFKA-7678: Avoid NPE when closing the RecordCollector Dec 3, 2018
Copy link
Copy Markdown
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

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

Overall LGTM. One nit.

Call for second review anyof @guozhangwang @bbejeck @vvcephei

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: use producer != null

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Member

@bbejeck bbejeck left a comment

Choose a reason for hiding this comment

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

LGTM as well modulo the comment from @mjsax

@mjsax mjsax added the streams label Dec 4, 2018
@jonathansantilli
Copy link
Copy Markdown
Contributor Author

Thanks for the review @mjsax @bbejeck, code updated as suggested :)
Cheers!

Copy link
Copy Markdown
Member

@bbejeck bbejeck left a comment

Choose a reason for hiding this comment

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

@jonathansantilli thanks for the update and the contribution!

@jonathansantilli
Copy link
Copy Markdown
Contributor Author

It was a pleasure @bbejeck, hope this goes to the master branch soon to test it.
Cheers!

@mjsax mjsax merged commit b616f91 into apache:trunk Dec 5, 2018
mjsax pushed a commit that referenced this pull request Dec 5, 2018
Reviewers: Matthias J. Sax <matthias@confluent.io>, Bill Bejeck <bill@confluent.io>
@guozhangwang
Copy link
Copy Markdown
Contributor

@jonathansantilli it's already merged to trunk by @mjsax , you can check out latest trunk head and try it out now :) Thanks for your contributions again!

mjsax pushed a commit that referenced this pull request Dec 5, 2018
Reviewers: Matthias J. Sax <matthias@confluent.io>, Bill Bejeck <bill@confluent.io>
mjsax pushed a commit that referenced this pull request Dec 5, 2018
Reviewers: Matthias J. Sax <matthias@confluent.io>, Bill Bejeck <bill@confluent.io>
@mjsax
Copy link
Copy Markdown
Member

mjsax commented Dec 5, 2018

Merged to trunk and cherry-picked to 2.1, 2.0, and 1.1 (not required for older versions).

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Dec 5, 2018

Thanks for the patch @jonathansantilli

@jonathansantilli
Copy link
Copy Markdown
Contributor Author

It was a pleasure @guozhangwang @mjsax

pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
Reviewers: Matthias J. Sax <matthias@confluent.io>, Bill Bejeck <bill@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants