Skip to content

KAFKA-9073: check assignment in requestFailed to avoid NPE#7630

Merged
guozhangwang merged 2 commits intoapache:2.3from
guozhangwang:K9073-check-assignment-to-avoid-npt
Dec 4, 2019
Merged

KAFKA-9073: check assignment in requestFailed to avoid NPE#7630
guozhangwang merged 2 commits intoapache:2.3from
guozhangwang:K9073-check-assignment-to-avoid-npt

Conversation

@guozhangwang
Copy link
Copy Markdown
Contributor

This is a cherry-pick of the bug-fix included in #6884 to 2.3 and older branch.

Committer Checklist (excluded from commit message)

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

@guozhangwang
Copy link
Copy Markdown
Contributor Author

@ableegoldman @hachikuji

Copy link
Copy Markdown
Member

@ableegoldman ableegoldman left a comment

Choose a reason for hiding this comment

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

Thanks for the quick PR, LGTM

By the way, I vaguely recall another minor bug you found during that long PR: Also found a minor bug in MetadataUpdate that removed topic would still be retained with null value of num.partitions.

I don't remember the specifics (got that from PR description) but is it worth tacking on to this PR as well? Doesn't seem like anyone has noticed that one or needs the fix, but it wouldn't hurt

@mjsax mjsax added the streams label Nov 1, 2019
@mjsax
Copy link
Copy Markdown
Member

mjsax commented Nov 1, 2019

Can we add a test for this case?

@guozhangwang
Copy link
Copy Markdown
Contributor Author

By the way, I vaguely recall another minor bug you found during that long PR: Also found a minor bug in MetadataUpdate that removed topic would still be retained with null value of num.partitions.

Ack, I think you were referring to this: https://github.com/apache/kafka/pull/6884/files/ddfe28f9dd5ce26f8f189946e5c5d90b627e33ca#r307557509 Will update this PR as well.

@guozhangwang
Copy link
Copy Markdown
Contributor Author

@mjsax I tried to add a unit test but this is not very straight-forward to test the exact root cause since it has to be a race condition between the assignment / subscription change and the request callback / metadata snapshot construction. I feel it is relatively safe to merge to old branches since it is a cherry-pick from trunk / 2.4. WDYT?

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.

@guozhangwang Feel free to merge.

@guozhangwang guozhangwang merged commit 44664cc into apache:2.3 Dec 4, 2019
@guozhangwang guozhangwang deleted the K9073-check-assignment-to-avoid-npt branch December 4, 2019 02:33
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.

3 participants