Implement Checkpoint::recover #3
Conversation
conanfile.py
Outdated
| self.requires("llfs/0.42.0", **VISIBLE) | ||
| self.requires("pcg-cpp/cci.20220409", **VISIBLE) | ||
| self.requires("vqf/0.2.5", **VISIBLE) | ||
| self.requires("vqf/0.2.5-devel", **VISIBLE) |
There was a problem hiding this comment.
Please revert, as discussed.
src/turtle_kv/checkpoint.cpp
Outdated
| std::shared_ptr<const TreeView> tree, | ||
| TreeView::from_page( | ||
| checkpoint_volume.cache().get_page(tree_root_id, llfs::OkIfNotFound{false}))); | ||
| // TODO: [Gabe Bornstein 11/4/25] Consider, error handling for invalid checkpoint |
There was a problem hiding this comment.
Maybe include some of the things to check, e.g. that the batch id is in the valid range.
There was a problem hiding this comment.
Now checking that batch_upper_bound is non-zero
src/turtle_kv/checkpoint.cpp
Outdated
|
|
||
| Subtree tree = Subtree::from_page_id(tree_root_id); | ||
|
|
||
| batt::StatusOr<i32> height = tree.get_height(*(checkpoint_volume.new_job())); |
There was a problem hiding this comment.
I think it's cheaper/better to call tree.get_height(checkpoint_volume.cache()) here.
| llfs::Volume& checkpoint_volume, | ||
| const llfs::SlotWithPayload<TabletCheckpoint>& checkpoint) noexcept; | ||
| #endif | ||
| static StatusOr<Checkpoint> recover(llfs::Volume& checkpoint_volume, |
There was a problem hiding this comment.
Please add doc comment.
src/turtle_kv/kv_store.cpp
Outdated
| return OkStatus(); | ||
| } | ||
|
|
||
| using CheckpointSlotPairs = std::vector<std::pair<llfs::SlotParse, turtle_kv::PackedCheckpoint>>; |
There was a problem hiding this comment.
Suggest either remove this indirection, or maybe append Vec to make readability better. Or consider replacing std::pair with a struct type.
There was a problem hiding this comment.
Restructured these functions and eliminated this type indirection
src/turtle_kv/kv_store.cpp
Outdated
|
|
||
| //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - | ||
| // | ||
| batt::StatusOr<CheckpointSlotPairs> recover_packed_checkpoints(llfs::Volume& volume) |
There was a problem hiding this comment.
Consider changing to something like StatusOr<PackedCheckpoint> recover_latest_packed_checkpoint, since we seem only to need the latest one anyhow.
There was a problem hiding this comment.
Compacted these two functions into a single function
|
Ready for another round of review! |
2 participants
No description provided.