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-6860: Fix NPE in Kafka Streams with EOS enabled #5187

Merged
merged 2 commits into from Jun 13, 2018

Conversation

@mjsax
Copy link
Member

mjsax commented Jun 11, 2018

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)
@mjsax mjsax added the streams label Jun 11, 2018
@mjsax mjsax requested a review from guozhangwang Jun 11, 2018
} else {
partitionsAndOffsets.put(storePartition, -1L);
}
partitionsAndOffsets.put(storePartition, checkpointableOffsets.getOrDefault(storePartition, -1L));

This comment has been minimized.

Copy link
@mjsax

mjsax Jun 11, 2018

Author Member

This is a Java8 rewrite only.

@mjsax

This comment has been minimized.

Copy link
Member Author

mjsax commented Jun 11, 2018

Call for review @bbejeck @vvcephei

PR also contains some code cleanup. This should be cherry-picked to 2.0 branch.

@mjsax

This comment has been minimized.

Copy link
Member Author

mjsax commented Jun 11, 2018

@guozhangwang I think, we should also cherry-pick to 0.11.0, 1.0, and 1.1 branches. WDYT?

Copy link
Contributor

vvcephei left a comment

Thanks for the patch.

streams/src/main/java/org/apache/kafka/streams/processor/internals/AbstractStateManager.java Outdated
log.error("Failed to write offset checkpoint file to {} while re-initializing {}: {}", checkpoint, stateStores, fatalException);
throw new StreamsException("Failed to reinitialize global store.", fatalException);

if (eosEnabled) {

This comment has been minimized.

Copy link
@bk-ko

bk-ko Jun 12, 2018

Sorry for interrupt. I guess you might miss ! in front of eosEnabled. Right ?

This comment has been minimized.

Copy link
@mjsax

mjsax Jun 12, 2018

Author Member

Great catch! Thanks for reviewing! (this happens when stupid me does not write a unit test...)

@mjsax

This comment has been minimized.

Copy link
Member Author

mjsax commented Jun 12, 2018

Updated this. Also added unit-test. Some more Java8 cleanup.

@bk-ko

This comment has been minimized.

Copy link

bk-ko commented Jun 12, 2018

It would be nice if this fix applies to 1.1!

@bbejeck

This comment has been minimized.

Copy link
Contributor

bbejeck commented Jun 12, 2018

retest this please

Copy link
Contributor

bbejeck left a comment

Thanks for the patch @mjsax, LGMT

Copy link
Contributor

guozhangwang left a comment

LGTM!

@guozhangwang

This comment has been minimized.

Copy link
Contributor

guozhangwang commented Jun 12, 2018

Assuming clients:checkstyleMain is fixed from jenkins, please feel free to merge and cherry-pick to old branches.

@mjsax mjsax force-pushed the mjsax:kafka-6860-npe-checkpoint-eos branch to 99b3999 Jun 13, 2018
@mjsax mjsax merged commit ff96d57 into apache:trunk Jun 13, 2018
mjsax added a commit that referenced this pull request Jun 13, 2018
Reviewers: John Roesler <john@confluent.io>, Ko Byoung Kwon, Bill Bejeck <bill@confluent.io>, Guozhang Wang <guozhang@confluent.io>
mjsax added a commit that referenced this pull request Jun 13, 2018
Reviewers: John Roesler <john@confluent.io>, Ko Byoung Kwon, Bill Bejeck <bill@confluent.io>, Guozhang Wang <guozhang@confluent.io>
@mjsax

This comment has been minimized.

Copy link
Member Author

mjsax commented Jun 13, 2018

Merged to trunk and cherry-picked to 2.0 and 1.1. (1.0 is no affected)

@mjsax mjsax deleted the mjsax:kafka-6860-npe-checkpoint-eos branch Jun 13, 2018
@bk-ko

This comment has been minimized.

Copy link

bk-ko commented Jun 14, 2018

Thank you for your quick fix!

ying-zheng added a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018
Reviewers: John Roesler <john@confluent.io>, Ko Byoung Kwon, Bill Bejeck <bill@confluent.io>, Guozhang Wang <guozhang@confluent.io>
nimosunbit added a commit to sunbit-dev/kafka that referenced this pull request Nov 6, 2018
Reviewers: John Roesler <john@confluent.io>, Ko Byoung Kwon, Bill Bejeck <bill@confluent.io>, Guozhang Wang <guozhang@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.