Skip to content

fix: correct end timestamp recovery in IndexStoreFile to read INDEX_END_TIME_STAMP instead of INDEX_BEGIN_TIME_STAMP - #10687

Open
waterWang wants to merge 1 commit into
apache:developfrom
waterWang:fix/index-storefile-end-timestamp-recovery
Open

fix: correct end timestamp recovery in IndexStoreFile to read INDEX_END_TIME_STAMP instead of INDEX_BEGIN_TIME_STAMP#10687
waterWang wants to merge 1 commit into
apache:developfrom
waterWang:fix/index-storefile-end-timestamp-recovery

Conversation

@waterWang

Copy link
Copy Markdown

Which Issue(s) This PR Fixes

Brief Description

The recovery constructor of IndexStoreFile reads endTimestamp from INDEX_BEGIN_TIME_STAMP (offset 4) instead of INDEX_END_TIME_STAMP (offset 12).

The write path (lines 192-193) correctly stores endTimestamp at INDEX_END_TIME_STAMP:

byteBuffer.putLong(INDEX_BEGIN_TIME_STAMP, this.beginTimestamp.get());
byteBuffer.putLong(INDEX_END_TIME_STAMP, this.endTimestamp.get());

But the recovery constructor (line 117) reads from the wrong offset:

// Before (bug):
this.endTimestamp.set(byteBuffer.getLong(INDEX_BEGIN_TIME_STAMP));

// After (fix):
this.endTimestamp.set(byteBuffer.getLong(INDEX_END_TIME_STAMP));

This causes the end timestamp to become the begin timestamp after reopening a tiered index file, which can cause valid query results to be incorrectly skipped.

How Did You Test This Change?

The fix is a one-line offset correction. The change is trivially verified by inspection: the constant INDEX_END_TIME_STAMP = 12 is used correctly in the write path but was incorrectly replaced with INDEX_BEGIN_TIME_STAMP = 4 in the recovery path. No functional logic change.

…ND_TIME_STAMP instead of INDEX_BEGIN_TIME_STAMP

@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

One-line bug fix: the recovery constructor of IndexStoreFile was reading endTimestamp from the wrong offset constant (INDEX_BEGIN_TIME_STAMP at offset 4 instead of INDEX_END_TIME_STAMP at offset 12). This caused endTimestamp to be incorrectly initialized to beginTimestamp's value during recovery.

Findings

  • [Critical] tieredstore/src/main/java/org/apache/rocketmq/tieredstore/index/IndexStoreFile.java:117 — This is a real correctness bug. The write path (lines 192-193) stores endTimestamp at INDEX_END_TIME_STAMP, but the recovery constructor read from INDEX_BEGIN_TIME_STAMP. The fix is correct and minimal.

  • [Info] No new test is added for this fix. Consider adding a test that creates an IndexStoreFile, writes data, then reconstructs via the recovery constructor and verifies that endTimestamp matches the original value (not beginTimestamp). This would prevent regression.

Overall

Clear, correct fix for a copy-paste bug. The change is minimal and precisely targets the issue. Would recommend adding a regression test if feasible. LGTM.


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

2 participants