-
Notifications
You must be signed in to change notification settings - Fork 473
Persist: Add new API for run based spine replacement #33044
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: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR adds a new API for run-based spine replacement in the persist client, extending the existing merge resolution functionality to support fine-grained replacement of specific runs within hollow batches rather than just entire batches.
- Adds
RunLocation
struct to identify specific runs within hollow batches usingSpineId
andRunId
pairs - Introduces a new
apply_merge_res_checked
method that can replace individual runs within batches - Extends
FueledMergeRes
withinputs
andnew_active_compaction
fields to support the new replacement logic
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
src/persist-client/src/internal/trace.rs | Core implementation of run-based spine replacement with new data structures and methods |
src/persist-client/src/internal/state_serde.json | Test data updates reflecting the new run-based structure |
src/persist-client/src/internal/state_diff.rs | Updates to populate new fields in FueledMergeRes |
src/persist-client/src/internal/state.rs | Test utilities and method renaming for compatibility |
src/persist-client/src/internal/machine.rs | Updates to construct FueledMergeRes with new fields |
src/persist-client/src/internal/compact.rs | Updates to construct FueledMergeRes with new fields |
src/persist-client/src/cli/admin.rs | Updates to construct FueledMergeRes with new fields |
} else { | ||
ApplyMergeResult::NotAppliedNoMatch | ||
} | ||
} | ||
|
||
#[allow(dead_code)] |
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.
It's not really dead!
f035a90
to
23e21c8
Compare
23e21c8
to
f497c4a
Compare
f497c4a
to
ae7312a
Compare
ae7312a
to
2bb9775
Compare
2bb9775
to
a15c651
Compare
a15c651
to
85910d2
Compare
85910d2
to
0274b1f
Compare
0274b1f
to
ee85994
Compare
ee85994
to
3c38325
Compare
3c38325
to
a97ff26
Compare
Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$T
mapping (possibly in a backwards-incompatible way), then it is tagged with aT-proto
label.