Skip to content

Conversation

@sjvanrossum
Copy link
Contributor

@sjvanrossum sjvanrossum commented Mar 7, 2025

Calls to poll have timed out if ConsumerRecords.empty() is (by reference) equal to the result of poll.
All other instances may have non-visible records at the end of a batch which should be skipped over while still claiming up to the next offset and updating the watermark.

Stacked on top of #34201.


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2025

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

@sjvanrossum sjvanrossum force-pushed the kafkaio-sdf-non-visible-progress branch from a3cbc4f to bf7e2d2 Compare March 14, 2025 16:40
@github-actions github-actions bot removed the build label Mar 14, 2025
@sjvanrossum
Copy link
Contributor Author

Run Java_Kafka_IO_Direct PreCommit

@sjvanrossum sjvanrossum force-pushed the kafkaio-sdf-non-visible-progress branch 2 times, most recently from d10f1a4 to 5da4e7f Compare March 20, 2025 12:03
@sjvanrossum sjvanrossum force-pushed the kafkaio-sdf-non-visible-progress branch from 5da4e7f to f81486e Compare March 21, 2025 13:16
@github-actions
Copy link
Contributor

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

R: @robertwb for label java.
R: @ahmedabu98 for label io.
R: @johnjcasey for label kafka.

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).

@chamikaramj
Copy link
Contributor

Any updates ?

@chamikaramj
Copy link
Contributor

@robertwb @ahmedabu98 or @johnjcasey PTAL.

@johnjcasey
Copy link
Contributor

LGTM

@github-actions
Copy link
Contributor

Reminder, please take a look at this pr: @robertwb @ahmedabu98 @johnjcasey

@github-actions
Copy link
Contributor

Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:

R: @kennknowles for label java.
R: @ahmedabu98 for label io.
R: @kennknowles for label kafka.

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)

@ahmedabu98
Copy link
Contributor

Hey @sjvanrossum, are you still working on this? do you need another review?

@sjvanrossum
Copy link
Contributor Author

@ahmedabu98 this is good to go, just needs another review. :)

@kennknowles
Copy link
Member

Latest code changes seem to have LGTM from John. Just needs merge.

@kennknowles
Copy link
Member

The commit descriptions look like they should be kept separate. Is that correct?

@sjvanrossum
Copy link
Contributor Author

sjvanrossum commented Apr 24, 2025

@kennknowles the second commit addresses an isolated issue (expected offset does not advance on the error handling path), but it only surfaced as a problem with the changes in the first commit of this PR.

@kennknowles kennknowles merged commit 46b2021 into apache:master Apr 24, 2025
18 checks passed
johnjcasey pushed a commit that referenced this pull request Apr 30, 2025
* Improve caching in backlog estimation and processing

* Add comment to explain the behavior of volatile guard field in KafkaLatestOffsetEstimator

* Guard against exceptions in endOffset refresh

* Call cancelIfTimeouted in roundtripElements to shutdown lingering pipelines

* Add missing calls to seek and/or pause before return points added in #34202
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.

5 participants