Skip to content

[client] fix: throw LogOffsetOutOfRangeException when scan offset ttl log #737

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

Merged
merged 5 commits into from
Apr 27, 2025

Conversation

gyang94
Copy link
Contributor

@gyang94 gyang94 commented Apr 11, 2025

Purpose

Linked issue: close #521

Brief change log

Based on comments in issue #521 , this PR did:

  1. remove the empty records check in LogFetcher, to pass the CompletedFetch (with error) into logFetcherBuffer

if (!MemoryLogRecords.EMPTY.equals(logRecords)) {

p.s. Based on the comments from @loserwang1024 , this empty records check may be necessary for some cases.

In oder to not signal notEmptyCondition, add completed fetch to buffer until log records is not empty.

I guess it is going to avoid several null records CompletedFetch (without error) in the first several polls.
But we can create a new issue ticket to resolve those cases.

  1. throw IllegalStateException when LogOffsetOutOfRangeException occurred in LogFetcherCollector, to show stack trace.

Tests

Add FlussLogITCase.
This test need to set a small ttl and wait for log clean up. It may not be good to reuse the cluster in existing IT cases. Thus add a new IT case for it.

API and Format

Documentation

@wuchong
Copy link
Member

wuchong commented Apr 18, 2025

Hi @gyang94 do you have time to update the PR? If not available in the recent days, I can help to update it and merge it.

@gyang94
Copy link
Contributor Author

gyang94 commented Apr 19, 2025

@wuchong Thanks for review. I will update it in this or next week.

@gyang94
Copy link
Contributor Author

gyang94 commented Apr 21, 2025

@wuchong updated. please check.

Copy link
Member

@wuchong wuchong left a comment

Choose a reason for hiding this comment

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

#588 has fixed this problem. So I rebased the branch and kept the tests to verify that the problem is fixed. Will merge it once CI passes.

@wuchong wuchong merged commit 1c77310 into apache:main Apr 27, 2025
3 checks passed
polyzos pushed a commit to polyzos/fluss that referenced this pull request Apr 27, 2025
ZmmBigdata pushed a commit to ZmmBigdata/fluss that referenced this pull request Jun 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subscribe from the log offset that has been ttl don't throw OffsetOutOfRangeException
2 participants