Snapshot module #164
Snapshot module #164
Conversation
| r.map(|(_, v)| v.old) | ||
| } else { | ||
| // otherwise, return current value | ||
| self.may_load(store, k) |
There was a problem hiding this comment.
What about signalling / exposing this difference? Particularly for Selected, it can be useful. At least for debugging.
Maybe by providing an alternative method, is_correct() or so; not to spoil the return value here.
There was a problem hiding this comment.
When the data is not available, I can either return possibly incorrect data, or return an error.
I think you are correct that an error is better.
That applies to any historical query for Selected::Never
There was a problem hiding this comment.
I addressed this, it down returns StdError::NotFound if there is no relevant checkpoint
There was a problem hiding this comment.
It is my understanding that it may apply to Selected as well: If you're using Selected, and didn't create / request a checkpoint for that specific height, you cannot tell for sure if the returned value is correct. It may be correct, but it may also be an updated value, which didn't get snapshotted at that particular height but at a later one. So, it may have been updated (and stored) at a later height (and we don't know what may have happened in between). Or, it's the current value, and we also cannot tell if it's still correct for that height.
There was a problem hiding this comment.
What I'm proposing is returning the value nevertheless, but return also a flag (or, maybe better, provide another method) to indicate if the value is guaranteed to be correct for that height, or not.
There was a problem hiding this comment.
I've seen you have implemented returning an error when the value is not guaranteed to be correct.
I prefer that, as it's less error prone: if you are using Selected and didn't get the guaranteed values for your heights, you're probably using it wrong. Same if you're not using a map with snapshots enabled, and pretend to get some historical values.
If the sub-key is the same type / size, this is already working, by the way. |
|
Thanks for the feedback, I will do some requested cleanup
Yeah, you have to store both separately, and I find height a much more natural (and guaranteed unique) measure than timestamp. There may be 3 blocks in the same second for example. I will leave that as an excercise for the reader. |
|
Fixed up change requests. I will merge when CI is happy. #165 builds on top and is another chance to review the module in practice. |
Closes #161
Make a SnapshotMap in
storage-plusthat handles this automatically for us.