-
Notifications
You must be signed in to change notification settings - Fork 522
fix(wal): filter inner trim record for range get #3000
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
Conversation
Signed-off-by: Robin Han <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug where trim marker records were incorrectly included in range query results from the Write-Ahead Log (WAL). The fix filters out internal trim records during batch reads to prevent them from being returned to users.
- Adds filtering logic to skip trim marker records (identified by
streamId == -1L,epoch == -1L, andcount == 0) during batch range reads - Adds a test case to verify trim records are correctly filtered when reading across a range that includes a trim marker
- Suppresses cyclomatic complexity warnings for the modified method
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| DefaultReader.java | Added logic to detect and filter out trim marker records during batch reads |
| ObjectWALServiceTest.java | Added test case to verify trim records are excluded from range query results |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| while (buf.readableBytes() > 0 && nextRecordOffset < readTask.endOffset.offset()) { | ||
| batches.add(ObjectUtils.decodeRecordBuf(buf)); | ||
| StreamRecordBatch batch = ObjectUtils.decodeRecordBuf(buf); | ||
| boolean isTriggerTrimRecord = batch.getCount() == 0 && batch.getStreamId() == -1L && batch.getEpoch() == -1L; |
Copilot
AI
Nov 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trim record detection logic is duplicated across the codebase. In RecoverIterator.java line 154, a similar check exists but only checks streamId == -1L && epoch == -1L, omitting the count == 0 check. Consider extracting this logic into a utility method (e.g., ObjectUtils.isTrimRecord()) to ensure consistency and maintainability across all trim record checks.
| boolean isTriggerTrimRecord = batch.getCount() == 0 && batch.getStreamId() == -1L && batch.getEpoch() == -1L; | |
| boolean isTriggerTrimRecord = ObjectUtils.isTrimRecord(batch); |
| import static com.automq.stream.s3.wal.impl.object.ObjectUtils.floorAlignOffset; | ||
| import static com.automq.stream.s3.wal.impl.object.ObjectUtils.genObjectPathV1; | ||
|
|
||
| @SuppressWarnings("checkstyle:cyclomaticComplexity") |
Copilot
AI
Nov 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cyclomatic complexity suppression is applied at the class level, which disables the check for all methods in the class. Consider applying this suppression more narrowly at the method level (e.g., on doRunBatchGet0 method) to maintain complexity checks for other methods.
Signed-off-by: Robin Han <[email protected]>
Signed-off-by: Robin Han <[email protected]>
Signed-off-by: Robin Han <[email protected]>
Signed-off-by: Robin Han <[email protected]>
No description provided.