Skip to content

KAFKA-14208; Do not raise wakeup in consumer during asynchronous offset commits#12626

Merged
showuon merged 3 commits intoapache:trunkfrom
hachikuji:K14208-do-not-wakeup-if-timed-out
Sep 13, 2022
Merged

KAFKA-14208; Do not raise wakeup in consumer during asynchronous offset commits#12626
showuon merged 3 commits intoapache:trunkfrom
hachikuji:K14208-do-not-wakeup-if-timed-out

Conversation

@hachikuji
Copy link
Copy Markdown
Contributor

@hachikuji hachikuji commented Sep 12, 2022 •
edited
Loading

Asynchronous offset commits may throw an unexpected WakeupException following #11631 and #12244. This patch fixes the problem by passing through a flag to ensureCoordinatorReady to indicate whether wakeups should be disabled. This is used to disable wakeups in the context of asynchronous offset commits. All other uses leave wakeups enabled.

Note: this patch builds on top of #12611.

Co-Authored-By: Guozhang Wang wangguoz@gmail.com

Committer Checklist (excluded from commit message)

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

@hachikuji hachikuji mentioned this pull request Sep 12, 2022
Closed
3 tasks
showuon
showuon approved these changes Sep 13, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

LGTM! Left a minor comment. And yes, I have to admit, it looks better! :)

Comment thread clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkClient.java Outdated
* @param disableWakeup true if we should not check for wakeups, false otherwise
*
* @return true if the future is done, false otherwise
* @throws WakeupException if {@link #wakeup()} is called from another thread
Copy link
Copy Markdown
Member

@showuon showuon Sep 13, 2022

Choose a reason for hiding this comment

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

We should mention WakeupException only throws when disableWakeup is false.

showuon
showuon approved these changes Sep 13, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

Thanks @hachikuji , I can monitor the build and merge it. And cherry-pick back to 3.3, 3.2.

@hachikuji
Copy link
Copy Markdown
Contributor Author

hachikuji commented Sep 13, 2022

@showuon Thanks! I appreciate it.

@hachikuji
Copy link
Copy Markdown
Contributor Author

hachikuji commented Sep 13, 2022

I have been seeing some recent build failures due to a non-zero exit code from core:unitTest. I cannot reproduce locally (of course) and this is making it tough to get some of these 3.3 patches over the line. I'll take a look tomorrow if I can, but if anyone has any ideas, do let me know.

@showuon
Copy link
Copy Markdown
Member

showuon commented Sep 13, 2022

I can take a look when I'm available today

@showuon
Copy link
Copy Markdown
Member

showuon commented Sep 13, 2022

Failed tests are unrelated:

    org.apache.kafka.common.network.SslTransportLayerTest.[1] tlsProtocol=TLSv1.2, useInlinePem=false
    kafka.test.ClusterTestExtensionsTest.[1] Type=ZK, Name=Generated Test, MetadataVersion=3.3-IV3, Security=PLAINTEXT
    org.apache.kafka.streams.processor.internals.StreamsAssignmentScaleTest.testHighAvailabilityTaskAssignorManyStandbys
    org.apache.kafka.trogdor.coordinator.CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated()

@showuon showuon merged commit 3d2ac7c into apache:trunk Sep 13, 2022
showuon pushed a commit that referenced this pull request Sep 13, 2022
@hachikuji @showuon
KAFKA-14208; Do not raise wakeup in consumer during asynchronous offs…
389bb20 
…et commits (#12626)

Asynchronous offset commits may throw an unexpected WakeupException following #11631 and #12244. This patch fixes the problem by passing through a flag to ensureCoordinatorReady to indicate whether wakeups should be disabled. This is used to disable wakeups in the context of asynchronous offset commits. All other uses leave wakeups enabled.

Note: this patch builds on top of #12611.

Co-Authored-By: Guozhang Wang wangguoz@gmail.com

Reviewers: Luke Chen <showuon@gmail.com>
showuon pushed a commit that referenced this pull request Sep 13, 2022
@hachikuji @showuon
KAFKA-14208; Do not raise wakeup in consumer during asynchronous offs…
e72db09 
…et commits (#12626)

Asynchronous offset commits may throw an unexpected WakeupException following #11631 and #12244. This patch fixes the problem by passing through a flag to ensureCoordinatorReady to indicate whether wakeups should be disabled. This is used to disable wakeups in the context of asynchronous offset commits. All other uses leave wakeups enabled.

Note: this patch builds on top of #12611.

Co-Authored-By: Guozhang Wang wangguoz@gmail.com

Reviewers: Luke Chen <showuon@gmail.com>
fmin added a commit to confluentinc/kafka that referenced this pull request Sep 14, 2022
@fmin
Merge remote-tracking branch 'apache-kafka/3.2' into sync-upstream-3.…
b687578 
…2-14-SEP-2022

* apache-kafka/3.2: (45 commits)
  MINOR: Bump version in upgrade guide to 3.2.3
  KAFKA-14208; Do not raise wakeup in consumer during asynchronous offset commits (apache#12626)
  KAFKA-14196; Do not continue fetching partitions awaiting auto-commit prior to revocation (apache#12603)
  MINOR: 3.2 branch version to 3.2.3-SNAPSHOT
  Bump version to 3.2.2
  Upgrade Netty and Jackson versions for CVE fixes [KAFKA-14044] (apache#12376)
  KAFKA-14194: Fix NPE in Cluster.nodeIfOnline (apache#12584)
  MINOR: Update LICENSE-binary
  MINOR: Align Scala version to 2.13.8
  MINOR: Bump version in upgrade guide to 3.2.2
  ...
@fmin fmin mentioned this pull request Sep 14, 2022
Merged
ijuma added a commit to confluentinc/kafka that referenced this pull request Sep 21, 2022
@ijuma
CONFLUENT: Sync from apache/kafka 3.3 to confluentinc/kafka 3.3 (21 S…
b084e90 
…eptember 2022)

`Jenkinsfile` was the only conflict and we ignore the changes since
they are not relevant to the Confluent build.

* apache-github/3.3: (61 commits)
  KAFKA-14214: Introduce read-write lock to StandardAuthorizer for consistent ACL reads. (apache#12628)
  KAFKA-14243: Temporarily disable unsafe downgrade (apache#12664)
  KAFKA-14240; Validate kraft snapshot state on startup (apache#12653)
  KAFKA-14233: disable testReloadUpdatedFilesWithoutConfigChange first to fix the build (apache#12658)
  KAFKA-14238;  KRaft metadata log should not delete segment past the latest snapshot (apache#12655)
  KAFKA-14156: Built-in partitioner may create suboptimal batches (apache#12570)
  MINOR: Adds KRaft versions of most streams system tests (apache#12458)
  MINOR; Add missing li end tag (apache#12640)
  MINOR: Mention that kraft is production ready in upgrade notes (apache#12635)
  MINOR: Add upgrade note regarding the Strictly Uniform Sticky Partitioner (KIP-794)  (apache#12630)
  KAFKA-14222; KRaft's memory pool should always allocate a buffer (apache#12625)
  KAFKA-14208; Do not raise wakeup in consumer during asynchronous offset commits (apache#12626)
  KAFKA-14196; Do not continue fetching partitions awaiting auto-commit prior to revocation (apache#12603)
  KAFKA-14215; Ensure forwarded requests are applied to broker request quota (apache#12624)
  MINOR; Remove end html tag from upgrade (apache#12605)
  Remove the html end tag from upgrade.html
  KAFKA-14205; Document how to replace the disk for the KRaft Controller (apache#12597)
  KAFKA-14203 Disable snapshot generation on broker after metadata errors (apache#12596)
  KAFKA-14216: Remove ZK reference from org.apache.kafka.server.quota.ClientQuotaCallback javadoc (apache#12617)
  KAFKA-14217: app-reset-tool.html should not show --zookeeper flag that no longer exists (apache#12618)
  ...
@ajborley ajborley mentioned this pull request Sep 26, 2022
Merged
@Baisang Baisang mentioned this pull request Feb 26, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@showuon showuon showuon approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hachikuji @showuon @guozhangwang