Skip to content

KAFKA-6748: double check before scheduling a new task after the punctuate call#4827

Merged
guozhangwang merged 3 commits intoapache:trunkfrom
fredfp:KAFKA-6748
Apr 5, 2018
Merged

KAFKA-6748: double check before scheduling a new task after the punctuate call#4827
guozhangwang merged 3 commits intoapache:trunkfrom
fredfp:KAFKA-6748

Conversation

@fredfp
Copy link
Copy Markdown
Contributor

@fredfp fredfp commented Apr 5, 2018

The first commit is a new test, illustrating the problem. The second commit fixes the problem.

Committer Checklist (excluded from commit message)

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

@fredfp
Copy link
Copy Markdown
Contributor Author

fredfp commented Apr 5, 2018

I can't add any watchers to the JIRA issue, so here we go.

@guozhangwang this is something I already noticed and we discussed earlier in the ML. I got bitten by this again and investigated further.

@mjsax Could you review please?

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

mjsax commented Apr 5, 2018

\cc @bbejeck @vvcephei

@guozhangwang guozhangwang changed the title fix for KAFKA-6748 KAFKA-6748: double check before scheduling a new task after the punctuate call Apr 5, 2018
guozhangwang
guozhangwang reviewed Apr 5, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@guozhangwang guozhangwang left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the catch.

Just a minor question about the test function.

Comment thread streams/src/test/java/org/apache/kafka/streams/processor/internals/PunctuationQueueTest.java Outdated
queue.mayPunctuate(now + 200L, PunctuationType.STREAM_TIME, processorNodePunctuator);
assertEquals(1, processor.punctuatedAt.size());

queue.mayPunctuate(now + 1001L, PunctuationType.STREAM_TIME, processorNodePunctuator);
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.

Why we want to test mayPunctuate(now + 1001L ..) again here?

Copy link
Copy Markdown
Contributor Author

@fredfp fredfp Apr 5, 2018

Choose a reason for hiding this comment

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

You are right, I just wanted to be 100% sure when writing the test, forgot to remove...

vvcephei
vvcephei approved these changes Apr 5, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@vvcephei vvcephei 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 PR!

@guozhangwang guozhangwang merged commit 3abd410 into apache:trunk Apr 5, 2018
guozhangwang pushed a commit that referenced this pull request Apr 5, 2018
@guozhangwang
KAFKA-6748: double check before scheduling a new task after the punct…
fc069bd 
…uate call (#4827)

After the punctuate() call, we would like to double check on the scheduled flag since the call itself may cancel it.

Reviewers: Guozhang Wang <wangguoz@gmail.com>, John Roesler <john@confluent.io>
@guozhangwang
Copy link
Copy Markdown
Contributor

guozhangwang commented Apr 5, 2018

Also cherry-picking to 1.1.

@fredfp
Copy link
Copy Markdown
Contributor Author

fredfp commented Apr 5, 2018

@guozhangwang Thanks! What about 1.0?

@guozhangwang
Copy link
Copy Markdown
Contributor

guozhangwang commented Apr 5, 2018 •
edited
Loading

I have cherry-picked to 1.0 as well, but we do not have another bugfix release plan for 1.x atm, just FYI. If there is a 1.0.2 bug fix release coming up later, it will include this fix.

guozhangwang pushed a commit that referenced this pull request Apr 5, 2018
@guozhangwang
KAFKA-6748: double check before scheduling a new task after the punct…
c029aac 
…uate call (#4827)

After the punctuate() call, we would like to double check on the scheduled flag since the call itself may cancel it.

Reviewers: Guozhang Wang <wangguoz@gmail.com>, John Roesler <john@confluent.io>
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)
  ...
ying-zheng pushed a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018
@ying-zheng
KAFKA-6748: double check before scheduling a new task after the punct…
918cdc1 
…uate call (apache#4827)

After the punctuate() call, we would like to double check on the scheduled flag since the call itself may cancel it.

Reviewers: Guozhang Wang <wangguoz@gmail.com>, John Roesler <john@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

@guozhangwang guozhangwang guozhangwang left review comments

@vvcephei vvcephei vvcephei approved these changes

@dguy dguy Awaiting requested review from dguy

@mjsax mjsax Awaiting requested review from mjsax

Assignees

No one assigned

Labels

streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fredfp @mjsax @guozhangwang @vvcephei