Skip to content

KAFKA-6747 Check whether there is in-flight transaction before aborting transaction#4826

Merged
guozhangwang merged 3 commits intoapache:trunkfrom
tedyu:trunk
Apr 5, 2018
Merged

KAFKA-6747 Check whether there is in-flight transaction before aborting transaction#4826
guozhangwang merged 3 commits intoapache:trunkfrom
tedyu:trunk

Conversation

@tedyu
Copy link
Copy Markdown
Contributor

@tedyu tedyu commented Apr 4, 2018

As Frederic reported on mailing list under the subject "kafka-streams Invalid transition attempted from state READY to state ABORTING_TRANSACTION", producer#abortTransaction should only be called when transactionInFlight is true.

Committer Checklist (excluded from commit message)

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

@tedyu tedyu changed the title Check whether there is in-flight transaction before aborting Check whether there is in-flight transaction before aborting transaction Apr 4, 2018
@tedyu tedyu changed the title Check whether there is in-flight transaction before aborting transaction KAFKA-6747 Check whether there is in-flight transaction before aborting transaction Apr 5, 2018
@tedyu
Copy link
Copy Markdown
Contributor Author

tedyu commented Apr 5, 2018

@mjsax
Can you review ?

@mjsax mjsax added the streams label Apr 5, 2018
@mjsax mjsax requested review from guozhangwang and mjsax April 5, 2018 15:32
@mjsax
Copy link
Copy Markdown
Member

mjsax commented Apr 5, 2018

Call for review @bbejeck @vvcephei

@guozhangwang
Copy link
Copy Markdown
Contributor

guozhangwang commented Apr 5, 2018

@tedyu Thanks for the PR, the fix looks good to me. Could we add a unit test to expose this issue and illustrate that it is fixed in this PR? For example:

  1. create a new StreamTask (beginTransaction not called yet).
  2. call task.close() immediately after the construction; without the fix it should be reproducing the error trace, and with the fix it should be fine.

guozhangwang
guozhangwang reviewed Apr 5, 2018
View reviewed changes
Comment thread streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamTask.java
if (!clean) {
try {
if (!isZombie) {
if (!isZombie && transactionInFlight) {
Copy link
Copy Markdown
Contributor

@guozhangwang guozhangwang Apr 5, 2018

Choose a reason for hiding this comment

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

This comment is not for this line but for line 361 above (it is not introduced in this PR but we can tighten the screws a bit more along side):

we should switch the line transactionInFlight = true; to go after producer.beginTransaction();

mjsax
mjsax reviewed Apr 5, 2018
View reviewed changes
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.

Thanks for the quick fix, @tedyu ! Just a minor comment about naming :)

Comment thread streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamTaskTest.java Outdated
}

@Test
public void noInitOnCreateWithEosEnabled() {
Copy link
Copy Markdown
Member

@mjsax mjsax Apr 5, 2018

Choose a reason for hiding this comment

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

nit: shouldNotThrowOnCloseIfTaskWasNotInitializedWithEosEnabled()

@guozhangwang guozhangwang merged commit ac542c9 into apache:trunk Apr 5, 2018
@mjsax
Copy link
Copy Markdown
Member

mjsax commented Apr 5, 2018

Should we cherry-pick this to 1.1 branch?

@guozhangwang
Copy link
Copy Markdown
Contributor

guozhangwang commented Apr 5, 2018

Also cherry-picked to 1.1 branch

guozhangwang pushed a commit that referenced this pull request Apr 5, 2018
@tedyu @guozhangwang
KAFKA-6747 Check whether there is in-flight transaction before aborti…
fd35336 
…ng transaction (#4826)

As Frederic reported on mailing list under the subject "kafka-streams Invalid transition attempted from state READY to state ABORTING_TRANSACTION", producer#abortTransaction should only be called when transactionInFlight is true.

Reviewers: Guozhang Wang <wangguoz@gmail.com>, Matthias J. Sax <matthias@confluent.io>
@colleen-love
Copy link
Copy Markdown

colleen-love commented Apr 21, 2018

Is there a workaround for this until 1.1.1 is released?

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Apr 24, 2018

I cannot think of any.

ijuma added a commit to confluentinc/kafka that referenced this pull request Apr 25, 2018
@ijuma
Merge remote-tracking branch 'confluent/1.1' into 4.1.x-with-interceptor
98c0a61 
* confluent/1.1: (60 commits)
  MINOR: Fix kafka-run-class for Java 10 (apache#4895)
  KAFKA-6772: Load credentials from ZK before accepting connections (apache#4867)
  KAFKA-6742: TopologyTestDriver error when dealing with stores from GlobalKTable
  MINOR: Mention that -1 disables retention by time (apache#4881)
  KAFKA-6790: Fix Streams processor node broken link (apache#4874)
  MINOR: Java 10 fixes so that the build passes (apache#4839)
  MINOR: Update Jackson to 2.9.5 (apache#4776)
  MINOR: Downgrade to Gradle 4.5.1 (apache#4791)
  MINOR: Java 9/10 fixes, gradle and minor deps update (apache#4725)
  KAFKA-6752: Enable unclean leader election metric (apache#4838)
  KAFKA-6054: Fix upgrade path from Kafka Streams v0.10.0 (apache#4773)
  KAFKA-6747 Check whether there is in-flight transaction before aborting transaction (apache#4826)
  KAFKA-6748: double check before scheduling a new task after the punctuate call (apache#4827)
  KAFKA-6739; Ignore headers when down-converting from V2 to V0/V1 (apache#4813)
  KAFKA-6728: Corrected the worker’s instantiation of the HeaderConverter
  KAFKA-6731: waitOnState should check the state to be the target start. (apache#4808)
  HOTFIX: Enforce a rebalance upon task migration (apache#4802)
  MINOR: Remove 1.2.0 changes from streams doc (apache#4784)
  MINOR: Update version numbers to 1.1.1-SNAPSHOT
  MINOR: Fix ReassignPartitionsClusterTest.testHwAfterPartitionReassignment test (apache#4781)
  ...
guozhangwang pushed a commit that referenced this pull request Jun 12, 2018
@tedyu @guozhangwang
KAFKA-6747 Check whether there is in-flight transaction before aborti…
a7000c4 
…ng transaction (#4826)

As Frederic reported on mailing list under the subject "kafka-streams Invalid transition attempted from state READY to state ABORTING_TRANSACTION", producer#abortTransaction should only be called when transactionInFlight is true.

Reviewers: Guozhang Wang <wangguoz@gmail.com>, Matthias J. Sax <matthias@confluent.io>
guozhangwang pushed a commit that referenced this pull request Jun 12, 2018
@tedyu @guozhangwang
KAFKA-6747 Check whether there is in-flight transaction before aborti…
9c9657b 
…ng transaction (#4826)

As Frederic reported on mailing list under the subject "kafka-streams Invalid transition attempted from state READY to state ABORTING_TRANSACTION", producer#abortTransaction should only be called when transactionInFlight is true.

Reviewers: Guozhang Wang <wangguoz@gmail.com>, Matthias J. Sax <matthias@confluent.io>
@guozhangwang
Copy link
Copy Markdown
Contributor

guozhangwang commented Jun 12, 2018

Also cherry-picked to 1.0 and 0.11.0

ying-zheng pushed a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018
@tedyu @ying-zheng
KAFKA-6747 Check whether there is in-flight transaction before aborti…
32a0ccd 
…ng transaction (apache#4826)

As Frederic reported on mailing list under the subject "kafka-streams Invalid transition attempted from state READY to state ABORTING_TRANSACTION", producer#abortTransaction should only be called when transactionInFlight is true.

Reviewers: Guozhang Wang <wangguoz@gmail.com>, Matthias J. Sax <matthias@confluent.io>
allenxwang pushed a commit to allenxwang/kafka that referenced this pull request Aug 24, 2018
Merge pull request apache#76 in DATA_PIPELINE/kafka from ~AWANG/kafka…
1ab0f95 
…:1.1-nflx to 1.1-nflx

* commit '84eeea7fe4b3a64b84b87d231969acfee4fb7544':
  Fix a bug where the ReqeustPerSec API version hash map is not updated.
  KAFKA-6772: Load credentials from ZK before accepting connections (apache#4867)
  KAFKA-6742: TopologyTestDriver error when dealing with stores from GlobalKTable
  MINOR: Mention that -1 disables retention by time (apache#4881)
  KAFKA-6790: Fix Streams processor node broken link (apache#4874)
  MINOR: Java 10 fixes so that the build passes (apache#4839)
  MINOR: Update Jackson to 2.9.5 (apache#4776)
  MINOR: Downgrade to Gradle 4.5.1 (apache#4791)
  MINOR: Java 9/10 fixes, gradle and minor deps update (apache#4725)
  KAFKA-6752: Enable unclean leader election metric (apache#4838)
  KAFKA-6054: Fix upgrade path from Kafka Streams v0.10.0 (apache#4773)
  KAFKA-6747 Check whether there is in-flight transaction before aborting transaction (apache#4826)
  KAFKA-6748: double check before scheduling a new task after the punctuate call (apache#4827)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mjsax mjsax mjsax left review comments

@guozhangwang guozhangwang guozhangwang approved these changes

Assignees

No one assigned

Labels

streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tedyu @mjsax @guozhangwang @colleen-love