Add slot deltas into the bank snapshot directory #29409
Conversation
xiangzhu70
force-pushed
the
add_slot_delta_into_bank_snapshot_dir
branch
from
ec592c6 to
d374097
Compare
There was a problem hiding this comment.
Biggest concern is forward/backward compatibility. It seems like the status_cache was moved. Did you manually test each of these cases?
- Old code loading a snapshot archive prepared w/ this change
- New code loading a snapshot archive prepared w/o this change
- New code loading a snapshot archive prepared w/ this change
The status_cache in the archive is the same. So the compatibility should not be an issue. It is just generated earlier in add_bank_snapshot(), and hard-linked into the archive staging directory, vs the old way of directly generated in the archive staging directory. |
xiangzhu70
force-pushed
the
add_slot_delta_into_bank_snapshot_dir
branch
from
343c1f1 to
a68ca25
Compare
There was a problem hiding this comment.
Need to write something here so that Github will mark this as "reviewed"
I think only #29409 (comment) is remaining.
github-actions
bot
added
the
stale
xiangzhu70
force-pushed
the
add_slot_delta_into_bank_snapshot_dir
branch
from
5d983ef to
d31c4c3
Compare
github-actions
bot
removed
the
stale
There was a problem hiding this comment.
Looking good! A few questions/nits below.
Also, now that the 1.15 branch has been created, you may need to rebase (according to: https://discord.com/channels/428295358100013066/439194979856809985/1070111773946024049)
xiangzhu70
force-pushed
the
add_slot_delta_into_bank_snapshot_dir
branch
from
6211469 to
cb7116a
Compare
There was a problem hiding this comment.
I'm happy with the change with a minor nit - leaving final approval to @brooksprumo since he's the expert in this area.
| let mut bank_snapshot_pre_path = bank_snapshot_post_path.clone(); | ||
| bank_snapshot_pre_path.set_extension(BANK_SNAPSHOT_PRE_FILENAME_EXTENSION); |
There was a problem hiding this comment.
This seems equivalent but w/o lingering mutability:
| let mut bank_snapshot_pre_path = bank_snapshot_post_path.clone(); | |
| bank_snapshot_pre_path.set_extension(BANK_SNAPSHOT_PRE_FILENAME_EXTENSION); | |
| let bank_snapshot_pre_path = bank_snapshot_post_path.with_extension(BANK_SNAPSHOT_PRE_FILENAME_EXTENSION); |
| let bank_snapshot_post_path = bank_snapshot_dir.join(get_snapshot_file_name(slot)); | ||
| let mut bank_snapshot_pre_path = bank_snapshot_post_path.clone(); |
There was a problem hiding this comment.
minor nit/optimization:
we only ever use the post path if the pre path does not exist. If we have a good way to directly get the pre path, it'd be better to check that, and only then generate the post path. Right now we have an extra PathBuf allocation.
Feel free to leave it as is for this PR
There was a problem hiding this comment.
Thanks! Agreed. I will make this change in the next PR. not worth rerun the auto tests just for this.
Problem
The slot_deltas is a necessary part of the bank state, needed for reconstructing the bank. It should be serialized together with the bank.
Summary of Changes
Serialize the slot_deltas into the bank snapshot directory in add_bank_snapshot, so it is generated with the bank file. Move it out of the AccountsPackage data structure. When building the archive, instead of using the data structure inside the AccountsPackage, hard-link the serialized slot-delta file in the bank snapshot directory.
This is one of the split PRs from #28745
Fixes #