-
Notifications
You must be signed in to change notification settings - Fork 537
sstable: add support for dual-tier blob handles #5713
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
1bc7efe to
5822dcb
Compare
Potential Bug(s) DetectedThe three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation. Next Steps: Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary. After you review the findings, please tag the issue as follows:
|
5822dcb to
cbaee20
Compare
Potential Bug(s) DetectedThe three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation. Next Steps: Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary. After you review the findings, please tag the issue as follows:
|
Previously, values existing in both hot and cold tiers had no mechanism to store both blob handles. This adds a coldBlobHandles column to the columnar data block format for storing cold blob handles when a value exists in both tiers. The new AddWithDualTierBlobHandles method writes entries with the hot handle in the value column and the cold handle in the dedicated column. The tiering histogram now tracks hot-and-cold blob reference bytes separately. Row-based writers return an error for dual-tier handles since they don't support the required columns
cbaee20 to
e2111f2
Compare
sumeerbhola
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sumeerbhola reviewed 14 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @annrpom and @xinhaoz).
-- commits line 11 at r1:
Is there a unit test that tries to retrieve the value using the secondary handle and validates that we get the same result, or will that come later when the read path is more ready?
sstable/colblk/data_block.go line 494 at r1 (raw file):
// secondaryBlobHandles stores secondary blob handles for dual-tier blob values. // This column is ONLY populated when a value exists in multiple tiers simultaneously. // For single-tier blobs (whether hot or cold), this column stores an empty slice.
Consider also saying that for dual-tier blob values the cold tier handle is placed here, so that normal accesses should use the cheaper access path of the primary blob handle.
sstable/colblk_writer.go line 537 at r1 (raw file):
eval.isObsolete = eval.isObsolete || forceObsolete w.prevPointKey.trailer = key.Trailer w.prevPointKey.isObsolete = eval.isObsolete
can we avoid duplicating a bunch of this code from AddWithBlobHandle and have both delegate to an internal helper, which does nothing with the secondaryHandle if it is empty?
Previously, values existing in both hot and cold tiers had no mechanism to store both blob handles.
This adds a coldBlobHandles column to the columnar data block format for storing cold blob handles when a value exists in both tiers. The new AddWithDualTierBlobHandles method writes entries with the hot handle in the value column and the cold handle in the dedicated column. The tiering histogram now tracks hot-and-cold blob reference bytes separately.
Row-based writers return an error for dual-tier handles since they don't support the required columns