[Enhancement] Add checksum protection for shared-data tablet metadata and txn log#74924
Conversation
|
No new undocumented parameters detected by the param-drift check. |
|
@codex review |
7c73cad to
8a87c13
Compare
8a87c13 to
e013a6c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c73cad15a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🌎 Translation Required?✅ All translation files are up to date.
|
e013a6c to
1ae76fe
Compare
82ee234 to
5e173b5
Compare
… and txn log Lake tablet metadata and txn logs were stored as plain protobuf with no checksum, so silent corruption could not be detected on read. - Add an opt-in checksummed format gated by BE config lake_enable_protobuf_file_checksum (default false; enable only after the whole cluster understands the format, to stay safe across rolling upgrade/downgrade). Readers always auto-detect and verify the checksum when present, regardless of the flag. - Single files (tablet meta, txn log/slog/vlog, combined txn log) use ProtobufFileWithHeader with a new LAKE_META_HEADER_MAGIC_NUMBER whose first on-disk byte is 0xFE, which no serialized protobuf can start with (wire type 6), so legacy headerless files are distinguished with zero false positives. - Bundle tablet metadata: per-tablet page checksum plus a footer crc32 over the shared header; the checksummed footer is marked by the high bit of the trailing size field (collision-free, since a legacy bundle size is well below 2^63), falling back to the legacy [bundle_meta][size] layout otherwise. - Route TabletManager and vacuum reads/writes through the new helpers, and adapt meta_tool (print_lake_metadata / print_lake_txn_log / print_lake_combined_txn_log) to parse both formats. - Add BE UTs for the single-file and bundle formats, legacy fallback, and corruption detection; update en/zh/ja BE config docs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: luohaha <18810541851@163.com>
5e173b5 to
15bd9d4
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
starrocks/be/src/storage/protobuf_file.cpp
Lines 172 to 175 in 15bd9d4
When lake_enable_protobuf_file_checksum is enabled, lake metadata/txn-log reads go through this buffer loader with legacy fallback enabled. A file containing a valid checksummed header/body plus extra trailing bytes now passes this < size check, so those bytes are neither covered by the Adler-32 nor rejected; the non-fallback streaming path previously detects the same condition by reading protobuf_length + 1 and failing on an oversized file. This masks append/trailing-byte corruption in the new checksummed format, so the reader should require the remaining size or header.file_length to match exactly before parsing.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[BE Incremental Coverage Report]✅ pass : 98 / 114 (85.96%) file detail
|
|
@Mergifyio backport branch-4.1 |
|
@Mergifyio backport branch-4.0 |
✅ Backports have been createdDetails
Cherry-pick of 77a3cd6 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
✅ Backports have been createdDetails
Cherry-pick of 77a3cd6 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
… and txn log (StarRocks#74924) Signed-off-by: luohaha <18810541851@163.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Oliver Layer <o.layer@celonis.com>
Why I'm doing:
In the shared-data architecture, lake tablet metadata and txn log files are
serialized as plain protobuf with no header and no checksum. Silent corruption of these
files (truncation, bit-rot, partial writes on object storage) cannot be detected on read,
and a corrupted tablet meta / txn log can lead to hard-to-diagnose failures.
The shared-nothing engine already protects its metadata with
ProtobufFileWithHeader,which carries an Adler-32 checksum, but that path hardcodes
OLAP_FIX_HEADER_MAGIC_NUMBER(== 0, a poor magic) and rejects anything else, so it cannot be reused as-is for lake.
What I'm doing:
Add an opt-in checksummed on-disk format for shared-data tablet metadata and txn logs,
with full backward/forward read compatibility.
LAKE_META_HEADER_MAGIC_NUMBER, chosen so its little-endian first on-diskbyte is
0xFE. A serializedTabletMetadataPB/TxnLogPBstarts with the field-1varint tag (
0x08) and protobuf can never produce a leading0xFEbyte, so a readercan distinguish the new checksummed format from legacy headerless protobuf with
effectively zero false-positive risk.
ProtobufFileWithHeaderwithmagicandallow_plain_protobuf_fallback(both defaulted), so existing shared-nothing callers are byte-for-byte unchanged. When
fallback is enabled,
load()parses legacy headerless protobuf when the magic is absent.lake_enable_protobuf_file_checksum(defaultfalse). It only controls thewrite format; readers always auto-detect and verify the checksum when present,
regardless of the flag.
save/load helpers in
TabletManager, and fixvacuum.cpp's own metadata reader to usethe same auto-detecting load path.
(
map<int64,uint32> tablet_meta_page_checksuminBundleTabletMetadataPB, same key astablet_meta_pages) plus a footer crc32 over the shared bundle header (schemas, mappings,page pointers, and the page-checksum map itself). When the flag is on the footer becomes
[bundle_meta][crc32][size | high-bit flag]; readers detect the layout via the high bit ofthe size field (collision-free, since a legacy bundle size is always well below 2^63) and
fall back to the legacy
[bundle_meta][size]when it is absent.meta_tool(print_lake_metadata/print_lake_txn_log/print_lake_combined_txn_log) to load via the header-aware path so it parses both formats;print_lake_bundle_metadataalready goes through the updatedparse_bundle_tablet_metadata.Compatibility note: the read path is always backward compatible (a new BE reads legacy
headerless files via fallback; the checksummed format is auto-detected). The flag defaults to
falseto stay safe during rolling upgrades and downgrades: while versions are mixed, an olderBE/CN uses the legacy reader and cannot parse newly-written checksummed files. Enable it only
after the whole cluster has been upgraded to a version that understands the format.
Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: