-
Notifications
You must be signed in to change notification settings - Fork 30
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
Make Snapshot
take a strategy trait so that users can write their own strategies
#82
base: release/1.2
Are you sure you want to change the base?
Make Snapshot
take a strategy trait so that users can write their own strategies
#82
Conversation
I just realized that since |
Hi!
I do appreciate the non-API-breaking way this was implemented. Is there a reason you're targetting the |
use super::{ChangeSet, SnapshotStrategy}; | ||
|
||
/// A SnapshotStrategy that takes a snapshot only if at least the specified interval has passed. | ||
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] |
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.
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] | |
#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] |
clippy complains here
This PR adds a
SnapshotStrategy
trait and adds a generic toSnapshot
,SnapshotMap
,SnapshotItem
, andIndexedSnapshotMap
. It also implements theSnapshotStrategy
trait for theStrategy
enum.This PR also adds a
IntervalStrategy
struct which is a strategy that only stores data to the changelog if at least a certain number of blocks has passed.I tried my best to make these changes non-breaking and I think I succeeded. However, I think there are more changes that could be make to simplify and improve
Snapshot
. For example, I found the "checkpointing" and the fnassert_checkpointed
quite confusing and it seems this is only used in theStrategy::Selected
variant. I couldn't figure out a way to move this logic into this variant without making breaking changes.I also found it a bit unintuitive that in
SnapshotMap
, data added to the changelog use the current height and not the height at which they were originally saved to theprimary
Map
. For the current purposes that I want to use SnapshotMap for it would make sense to use the height at which the data was originally saved, so that the changelog becomes a record of history rather than having a delay in the heights. Would you be interested in me adding an alternative implementation that accomplishes this? Trying to modify the current implementation would cause breaking changes.