Skip to content

chore(storage): require 12-byte TFChunk posmaps#155

Merged
yordis merged 1 commit intomasterfrom
yordis/chore-remove-legacy-posmap
May 3, 2026
Merged

chore(storage): require 12-byte TFChunk posmaps#155
yordis merged 1 commit intomasterfrom
yordis/chore-remove-legacy-posmap

Conversation

@yordis
Copy link
Copy Markdown
Member

@yordis yordis commented May 3, 2026

  • Removes legacy chunk-map compatibility so storage behavior reflects the current supported on-disk format instead of preserving an obsolete fallback path.

@cursor
Copy link
Copy Markdown

cursor Bot commented May 3, 2026

PR Summary

Medium Risk
Drops the legacy on-disk posmap/footers format, so nodes with older chunk files may now fail to read/boot until data is upgraded; changes touch chunk read/write paths and footer parsing.

Overview
Storage now requires 12-byte TFChunk position maps and removes the deprecated 8-byte compatibility path.

ChunkFooter no longer carries an IsMap12Bytes mode: it always serializes/deserializes LogicalDataSize as long, always sets the 12-byte map flag when formatting, and throws when parsing a completed footer missing that flag. PosMap drops old-format parsing helpers, and scavenged-chunk read logic (TFChunkReadSide) and footer creation/writing (TFChunk, test DB utilities) are updated to assume PosMap.FullSize only, with tests adjusted and new coverage added for rejecting deprecated footers.

Reviewed by Cursor Bugbot for commit 137907b. Bugbot is set up for automated code reviews on this repo. Configure here.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 3, 2026

Warning

Rate limit exceeded

@yordis has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 42 minutes and 54 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c51327ff-76bb-4a3e-b8e0-4c607bda6211

📥 Commits

Reviewing files that changed from the base of the PR and between d134e82 and 137907b.

📒 Files selected for processing (7)
  • src/EventStore.Core.Tests/TransactionLog/Validation/TFChunkDbCreationTestBase.cs
  • src/EventStore.Core.XUnit.Tests/TransactionLog/Chunks/ChunkFooterTests.cs
  • src/EventStore.Core.XUnit.Tests/TransactionLog/Chunks/PosMapTests.cs
  • src/EventStore.Core/TransactionLog/Chunks/ChunkFooter.cs
  • src/EventStore.Core/TransactionLog/Chunks/TFChunk/PosMap.cs
  • src/EventStore.Core/TransactionLog/Chunks/TFChunk/TFChunk.cs
  • src/EventStore.Core/TransactionLog/Chunks/TFChunk/TFChunkReadSide.cs

Walkthrough

This PR removes deprecated 12-byte position map format support from the transaction log chunk infrastructure. The ChunkFooter constructor is simplified to remove the isMap12Bytes parameter, PosMap.FromOldFormat methods are deleted, and all downstream code is updated to handle only the new 16-byte map format.

Changes

Position Map Format Deprecation

Layer / File(s) Summary
Constructor Signature
src/EventStore.Core/TransactionLog/Chunks/ChunkFooter.cs
ChunkFooter primary constructor removes isMap12Bytes parameter; MapSize validation and MapCount computation now use PosMap.FullSize exclusively.
Parsing & Format Methods
src/EventStore.Core/TransactionLog/Chunks/ChunkFooter.cs
Span-based parsing constructor rejects deprecated 8-byte position maps and reads LogicalDataSize unconditionally as long. Format method always sets the position-map format flag and writes LogicalDataSize as long.
Position Map Implementation
src/EventStore.Core/TransactionLog/Chunks/TFChunk/PosMap.cs
FromOldFormat methods and DeprecatedSize constant are removed; parsing surface reduced to new-format constructors and IBinaryFormattable<PosMap>.Parse.
Footer Construction
src/EventStore.Core/TransactionLog/Chunks/TFChunk/TFChunk.cs
WriteFooter constructs footerNoHash with updated ChunkFooter signature (removes second boolean argument) and formats it before MD5 computation.
Position Map Reading
src/EventStore.Core/TransactionLog/Chunks/TFChunk/TFChunkReadSide.cs
PopulateMidpoints and ReadPosMap overloads remove IsMap12Bytes branching; all map indexing uses PosMap.FullSize and decoding uses FromNewFormat / ReadAsync<PosMap>.
Test Infrastructure & Tests
src/EventStore.Core.Tests/TransactionLog/Validation/TFChunkDbCreationTestBase.cs, src/EventStore.Core.XUnit.Tests/TransactionLog/Chunks/ChunkFooterTests.cs, src/EventStore.Core.XUnit.Tests/TransactionLog/Chunks/PosMapTests.cs
DbUtil helpers updated to use new ChunkFooter constructor. ChunkFooterTests.can_round_trip refactored to single-parameter theory and IsMap12Bytes assertion removed. PosMapTests.can_parse_old_format_from_span test deleted; new rejects_deprecated_position_map_footer test added.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • PR #91: Modifies ChunkFooter parsing, footer reading paths, and position-map format handling; shares direct code-level overlap in footer and chunk metadata logic.
  • PR #74: Removes old-format PosMap.FromOldFormat handling and related position-map parsing branches; directly addresses the same deprecated format removal.
  • PR #65: Updates TFChunk.WriteFooter construction and footer serialization; shares direct changes in the same method being modified here.

Poem

🐰 Deprecated maps, be gone, be gone!
We hop to new formats, sixteen bytes strong.
No more the branching, the ifs, the old ways—
Just clean, simple code for all of our days! 🌱

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: removing legacy 8-byte position map support and requiring the 12-byte format, which is the primary refactoring across multiple storage-related files.
Description check ✅ Passed The description is directly related to the changeset, explaining that legacy chunk-map compatibility is being removed to reflect current supported on-disk format.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch yordis/chore-remove-legacy-posmap

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 42 minutes and 54 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
@yordis yordis force-pushed the yordis/chore-remove-legacy-posmap branch from d134e82 to 137907b Compare May 3, 2026 04:22
@yordis yordis merged commit 21a0e0c into master May 3, 2026
18 checks passed
@yordis yordis deleted the yordis/chore-remove-legacy-posmap branch May 3, 2026 04:37
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.

1 participant