lsm/sst: read bloom filters lazily#31008
Open
andrwng wants to merge 1 commit into
Open
Conversation
The bloom filter scales with the SST: ~1.5% of the file. We loaded the whole filter at open and pinned it for the reader's lifetime, so resident memory grew with (open readers x SST size). That's fine for L0 but doesn't hold as the metastore fills deeper levels, where a single SST's filter runs to hundreds of MiB. open() now retains the per-region offset array and reads each region from the file on demand. In order to still validate the on-disk contents, open() will read the whole bloom filter and validate the overall CRC, and also keep track of a not-persisted CRC of each filter region. Upon loading regions from disk thereafter, the region CRCs are validated. A test is added that corrupts a region byte after open and asserts the lazy read is rejected; sst_test covers the open-time whole-block check. NOTE: open IO is unchanged; the win is resident memory, not fewer reads.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes SST bloom-filter handling to reduce resident memory growth by lazily reading bloom filter regions on demand, while preserving integrity checks via an open-time whole-block CRC and per-region CRC validation on subsequent reads.
Changes:
- Make
lsm::block::filter_readerlazily read filter regions from the backingrandom_access_file_readerand validate each region against a CRC computed at open. - Update
lsm::sst::readerto use the new async filter reader API (filter_reader::open/ asynckey_may_match). - Add/extend tests to verify corruption is detected both at SST open time (whole-block CRC) and during lazy region reads (per-region CRC).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/v/lsm/sst/tests/sst_test.cc | Adds an SST-level corruption test that flips a filter byte and expects open-time CRC validation to fail. |
| src/v/lsm/sst/tests/BUILD | Adds deps needed by the new SST corruption test utilities. |
| src/v/lsm/sst/reader.cc | Switches filter loading to filter_reader::open and awaits async key_may_match. |
| src/v/lsm/block/tests/filter_test.cc | Reworks tests to exercise lazy region reads and validate per-region CRC detection after open. |
| src/v/lsm/block/tests/BUILD | Updates deps for new filter tests (crc32c, ioarray, exceptions). |
| src/v/lsm/block/filter.h | Replaces eager in-memory filter reader with lazy async API and documents integrity approach. |
| src/v/lsm/block/filter.cc | Implements lazy region reads plus open-time whole-block CRC and per-region CRC computation/validation. |
| src/v/lsm/block/BUILD | Adds implementation deps needed for CRC/compression/exceptions in the new filter reader. |
Comment on lines
+47
to
+50
| ss::future<ioarray> read(size_t offset, size_t n) override { | ||
| n = std::min(n, _data.size_bytes() - offset); | ||
| co_return ioarray::copy_from(_data.share(offset, n)); | ||
| } |
Comment on lines
+42
to
+45
| ss::future<ioarray> read(size_t offset, size_t n) override { | ||
| n = std::min(n, _data.size_bytes() - offset); | ||
| co_return ioarray::copy_from(_data.share(offset, n)); | ||
| } |
Comment on lines
+53
to
+66
| void corrupt_byte(size_t offset) { | ||
| std::string buf(_data.size_bytes(), '\0'); | ||
| auto* p = buf.data(); | ||
| iobuf::iterator_consumer(_data.cbegin(), _data.cend()) | ||
| .consume(buf.size(), [&p](const char* src, size_t sz) { | ||
| std::memcpy(p, src, sz); | ||
| p += sz; | ||
| return ss::stop_iteration::no; | ||
| }); | ||
| buf[offset] = static_cast<char>(~static_cast<uint8_t>(buf[offset])); | ||
| iobuf out; | ||
| out.append(buf.data(), buf.size()); | ||
| _data = std::move(out); | ||
| } |
Collaborator
CI test resultstest results on build#86688
|
andrwng
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bloom filter scales with the SST: ~1.5% of the file. We loaded the whole filter at open and pinned it for the reader's lifetime, so resident memory grew with (open readers x SST size). That's fine for L0 but doesn't hold as the metastore fills deeper levels, where a single SST's filter runs to hundreds of MiB.
open() now retains the per-region offset array and reads each region from the file on demand. In order to still validate the on-disk contents, open() will read the whole bloom filter and validate the overall CRC, and also keep track of a not-persisted CRC of each filter region. Upon loading regions from disk thereafter, the region CRCs are validated.
A test is added that corrupts a region byte after open and asserts the lazy read is rejected; sst_test covers the open-time whole-block check.
NOTE: open IO is unchanged; the win is resident memory, not fewer reads.
Backports Required
Release Notes