Skip to content

[ISSUE #10656] Fix incorrect end timestamp recovery in tiered index files#10657

Open
majialoong wants to merge 2 commits into
apache:developfrom
majialoong:ISSUE-10656
Open

[ISSUE #10656] Fix incorrect end timestamp recovery in tiered index files#10657
majialoong wants to merge 2 commits into
apache:developfrom
majialoong:ISSUE-10656

Conversation

@majialoong

Copy link
Copy Markdown
Contributor

Which Issue(s) This PR Fixes

Brief Description

Fix IndexStoreFile to restore endTimestamp from INDEX_END_TIME_STAMP and add a regression test.

How Did You Test This Change?

Added a test that writes an index, reopens the file, and verifies that its end timestamp is recovered correctly with and without mmap writes.

@RockteMQ-AI RockteMQ-AI left a comment

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.

Review by github-manager-bot

Summary

Fixes a copy-paste bug in IndexStoreFile constructor where endTimestamp was incorrectly recovered from INDEX_BEGIN_TIME_STAMP (offset 4) instead of INDEX_END_TIME_STAMP (offset 12).

Findings

  • [Info] IndexStoreFile.java:117 — The fix is correct. The write path (lines 192-193) stores beginTimestamp at INDEX_BEGIN_TIME_STAMP and endTimestamp at INDEX_END_TIME_STAMP. The old read was using the wrong offset, meaning after recovery, endTimestamp would hold the same value as beginTimestamp, breaking time-range queries on recovered index files.

  • [Info] IndexStoreFileTest.java — Good regression test. It writes an index entry with a distinct endTimestamp, reopens the file, and verifies recovery. Consider also testing the edge case where endTimestamp == beginTimestamp (single-entry file) to ensure no off-by-one.

  • [Info] The fix is minimal (1-line source change + test) and directly addresses the root cause. No compatibility concerns since this is a bug fix in the recovery path — the on-disk format is unchanged.

Verdict

Clean fix with proper test coverage. LGTM.


Automated review by github-manager-bot

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.21%. Comparing base (577b89f) to head (11b5aae).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop   #10657      +/-   ##
=============================================
- Coverage      48.32%   48.21%   -0.11%     
+ Complexity     13490    13464      -26     
=============================================
  Files           1380     1380              
  Lines         101031   101044      +13     
  Branches       13092    13094       +2     
=============================================
- Hits           48825    48721     -104     
- Misses         46247    46341      +94     
- Partials        5959     5982      +23     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@RockteMQ-AI RockteMQ-AI left a comment

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.

Review by github-manager-bot

Summary

Fixes incorrect endTimestamp recovery in tiered store index files — the constructor was reading from INDEX_BEGIN_TIME_STAMP offset instead of INDEX_END_TIME_STAMP when restoring the end timestamp from the mapped file buffer.

Findings

  • [Critical] IndexStoreFile.java:117 — The original code reads endTimestamp from the wrong buffer offset (INDEX_BEGIN_TIME_STAMP). This means after recovery, the index file's endTimestamp would be set to the begin timestamp, breaking any logic that depends on accurate end timestamps (e.g., compaction decisions, time-range queries).
  • [Info] IndexStoreFileTest.java — New recoverEndTimestampTest correctly validates the fix by writing an entry with a known end timestamp, shutting down, recovering, and asserting the end timestamp is preserved.

Verdict

Clean one-line bug fix with appropriate test coverage. The fix is clearly correct — the variable name endTimestamp should be initialized from INDEX_END_TIME_STAMP, not INDEX_BEGIN_TIME_STAMP.


Automated review by github-manager-bot

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] Incorrect end timestamp recovery in tiered index files

3 participants