This repository has been archived by the owner on Jan 22, 2025. It is now read-only.
Add base_slot to get_snapshot_storages() #19348
Merged
Conversation
This file contains 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
brooksprumo
commented
Aug 20, 2021
| @@ -788,7 +788,7 @@ mod tests { | |||
| .find(|elem| elem.slot == slot) | |||
| .ok_or_else(|| Error::new(ErrorKind::Other, "did not find snapshot with this path"))?; | |||
| let storages = { | |||
| let mut storages = bank.get_snapshot_storages(); | |||
| let mut storages = bank.get_snapshot_storages(Some(incremental_snapshot_base_slot)); | |||
| snapshot_utils::filter_snapshot_storages_for_incremental_snapshot( | |||
There was a problem hiding this comment.
The call to filter_snapshot_storages_for_incremental_snapshot() will be removed in the next PR (#19349). I am trying to keep PRs as minimal as possible :)
brooksprumo
commented
Aug 20, 2021
Comment on lines
-487
to
-492
| pub fn get_snapshot_storages(&self, slot: Slot) -> SnapshotStorages { | ||
| self.accounts | ||
| .accounts_db | ||
| .get_snapshot_storages(slot, None) | ||
| .0 | ||
| } |
There was a problem hiding this comment.
This function is never called, except by Bank below, so I've just moved it there.
brooksprumo
commented
Aug 20, 2021
| @@ -1636,7 +1636,7 @@ pub fn bank_to_incremental_snapshot_archive( | |||
|
|
|||
| let temp_dir = tempfile::tempdir_in(snapshots_dir)?; | |||
| let storages = { | |||
| let mut storages = bank.get_snapshot_storages(); | |||
| let mut storages = bank.get_snapshot_storages(Some(full_snapshot_slot)); | |||
| filter_snapshot_storages_for_incremental_snapshot(&mut storages, full_snapshot_slot); | |||
There was a problem hiding this comment.
The call to filter_snapshot_storages_for_incremental_snapshot() will be removed in the next PR (#19349).
carllin
approved these changes
Aug 20, 2021
Codecov Report
@@ Coverage Diff @@
## master #19348 +/- ##
=========================================
- Coverage 82.8% 82.8% -0.1%
=========================================
Files 457 457
Lines 130654 130690 +36
=========================================
+ Hits 108282 108290 +8
- Misses 22372 22400 +28 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Problem
When taking a bank snapshot that is destined to be an incremental snapshot, we collect all the snapshot storages, then immediately filter/throw out the ones below the incremental snapshot base slot (aka last full snapshot slot). Ideally, we'd only collect the snapshot storages that we needed in the first place.
Summary of Changes
Add a
snapshot_base_slotoptional parameter toget_snapshot_storages(), and filter/retain only slots higher.Related to #17088