Snapshot item #409
Snapshot item #409
Conversation
maurolacy
force-pushed
the
snapshot-item
branch
from
ff01f3e
to
31f8a84
Compare
|
I'm doing a deeper dig through the code now. The diffs don't help, I need to check it out. The first comment is with snapshot.rs, snapshot_map.rs and snapshot_item.rs, maybe they deserve a submodule. This package is growing and could use some structure. Like Maybe index code could also be grouped like this as well, but that is a different PR. |
There was a problem hiding this comment.
Very nice design and well done pulling out the Snapshot.
I propose 2 more methods to put into Snapshot.
And it would be nice to have a few basic unit tests in snapshot.rs (I trust it as it works in higher level, but to demo the API and hit a few edge cases, it would be nice).
Other than that, good to merge IMO
| /// load old value and store changelog | ||
| fn write_change(&self, store: &mut dyn Storage, height: u64) -> StdResult<()> { | ||
| // if there is already data in the changelog for this block, do not write more | ||
| if self |
There was a problem hiding this comment.
likewise, a helper here, like:
self.snapshots.has_changelog(store, (), height) would be nice.
There was a problem hiding this comment.
And maybe expose write_change on self.snapshots, as this same basic logic is used for Map
|
OK, will refactor. Yes, diffs don't help anymore. What you can do is read / review Finally, I can separate these into two / three PRs for ease of reviewing:
Hadn't seen your latest comments. Let's put this last one into a follow-up. |
|
I just checked out the code and read it. It looked good. No need to break it up. Just saw these 2 non-trivial methods I figured could move into snapshot, then they all look great. Moving the files and even adding more unit tests could be done in a separate PR. Happy to have this. |
maurolacy
force-pushed
the
snapshot-item
branch
from
b781e6a
to
505be01
Compare
|
Please take a look. I would merge this as it is, and then do a follow-up with the paths refactoring, and adding some |
| // if we found a match, return this last one | ||
| r.map(|(_, v)| Some(v.old)) | ||
| } else { | ||
| Ok(None) |
| r.map(|(_, v)| v.old) | ||
| let snapshot = self.snapshots.may_load_at_height(store, (), height)?; | ||
|
|
||
| if let Some(r) = snapshot { |
There was a problem hiding this comment.
Yes, this looks nice here, keeping the primary load in this function, and checking an option to switch.
Closes #193.
Straightforward impl adapting fromCreated aSnapshotMap.Snapshotabstr, and used it inSnapshotItemandSnaphotMap.Still need to review / validate tests.(done in #418)