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

[bug30870]: make consumer polling timeout configurable for KafkaIO.Read #30877

Merged
merged 9 commits into from
Apr 9, 2024

Conversation

xianhualiu
Copy link
Contributor

addresses #30870. The changes in this PR make the consumer polling timeout configurable for KafkaIO.Read with following new command:

KafkaIO.read().withConsumerPollingTimeout(Duration duration)

The duration must be greater than zero. If not specified, the default will be 1 second.

Copy link
Contributor

github-actions bot commented Apr 5, 2024

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

Copy link
Contributor

@jbsabbagh jbsabbagh left a comment

Choose a reason for hiding this comment

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

Thanks @xianhualiu - the API looks good!

I added some comments on maybe increasing the default + some additional logging to inform users of this timeout. This issue being so obscure and difficult to debug, I worry that users will likely not realize that they should increase the transform's poll timeout.

Additionally, this issue also affects the Python SDK. Are you intending to include a fix to the Python API in this PR or in a follow up PR?

Copy link
Contributor

@jbsabbagh jbsabbagh left a comment

Choose a reason for hiding this comment

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

LGTM!

updated changes.md with changes to make consumer polling timeout configurable for KafkaIO.Read
CHANGES.md Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Apr 8, 2024

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @bvolpato for label java.
R: @bvolpato for label io.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

added break changes
CHANGES.md Outdated Show resolved Hide resolved
@Abacn
Copy link
Contributor

Abacn commented Apr 15, 2024

It appears this breaks Java IOs PreCommit: #30941 please fix

@Abacn
Copy link
Contributor

Abacn commented Apr 15, 2024

Also as followup :sdks:java:io:kafka:upgrade:test should be exercised in Kafka IO PreCommit

Abacn added a commit to Abacn/beam that referenced this pull request Apr 16, 2024
@xianhua
Copy link

xianhua commented Oct 1, 2024

This PR has merged into master and the revert change by Yi Hu never merged since the test issues have been fixed. The PR containing the revert is cloded now: #31001.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants