Skip to content

[server] Update remote log offsets before local segment cleanup - #4254

Open
loserwang1024 wants to merge 2 commits into
apache:mainfrom
loserwang1024:fix-remote-log-offset-update
Open

[server] Update remote log offsets before local segment cleanup#4254
loserwang1024 wants to merge 2 commits into
apache:mainfrom
loserwang1024:fix-remote-log-offset-update

Conversation

@loserwang1024

Copy link
Copy Markdown
Contributor

Purpose

Linked issue: close #4247

Brief change log

Tests

API and Format

Documentation

@loserwang1024 loserwang1024 self-assigned this Sep 8, 2026
@loserwang1024
loserwang1024 force-pushed the fix-remote-log-offset-update branch from d155b78 to b63f1c3 Compare September 8, 2026 12:08
// deletes the local segment containing offset 25.
logTablet.updateRemoteLogEndOffset(40L, 40L);
assertThat(logTablet.localLogStartOffset()).isEqualTo(30L);
assertThat(logTablet.canFetchFromRemoteLog(25L)).isTrue();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here we checked the final state after updateRemoteLogEndOffset. It can't check whether we have the correct update processing order. We must 1) update copied watermark first then 2) delete local segment.
The problem is even if we first do operation 2 then 1, this test will also pass.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

t's hard to test this unless we add a hook before cleanupSegments, which is too intrusive to the production code.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sure. It is not a blocker.

@loserwang1024
loserwang1024 force-pushed the fix-remote-log-offset-update branch from b63f1c3 to a496a31 Compare September 9, 2026 03:15
@gyang94

gyang94 commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

@loserwang1024 would it be better to update remote log offset in one single function? (e.g. updateRemoteLogOffsets(start, end, highestCopied)). Now we have two functions updateRemoteLogStartOffset and updateRemoteLogEndOffset. But there is a hidden requirement is that we must call this two functions at the same time to update remote start/end offset together. Thus use one single function may be better.

This is not a blocker either, just a code optimization.

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.

[Bug] Fetch may fail during non-atomic remote log offset updates

2 participants