Make state value metadata upgradable at runtime #11333
Conversation
msmouse
requested review from
lightmark,
JoshLind,
gregnazario,
gelash,
zekun000,
sasha8,
danielxiangzl,
alinush,
davidiw,
wrwg and
georgemitenkov
as code owners
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
msmouse
force-pushed
the
1211-alden-metadata-upgrade
branch
from
cadf37d
to
c85dde8
Compare
msmouse
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #11333 +/- ##
=========================================
- Coverage 68.7% 68.7% -0.1%
=========================================
Files 766 766
Lines 177267 177406 +139
=========================================
+ Hits 121893 121917 +24
- Misses 55374 55489 +115 ☔ View full report in Codecov by Sentry. |
msmouse
force-pushed
the
1211-alden-metadata-upgrade
branch
5 times, most recently
from
49d7665
to
7113866
Compare
msmouse
force-pushed
the
1211-alden-metadata-upgrade
branch
from
7113866
to
76d2d3d
Compare
msmouse
added
the
CICD:run-e2e-tests
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
types/src/state_store/state_value.rs
Outdated
| StateValueInner::V0(bytes) => (None, bytes), | ||
| StateValueInner::WithMetadata { data, metadata } => (Some(metadata), data), | ||
| } | ||
| pub fn into_parts(self) -> (StateValueMetadata, Bytes) { |
There was a problem hiding this comment.
nit: this is typically called unpack?
There was a problem hiding this comment.
I think we call it into_inner() throughout the code :)
There was a problem hiding this comment.
... Just changed to unpack.. 
We have more unpack and into_inner than into_parts, to be fair, but because this doesn't have an inner, I like unpack more.
This comment has been minimized.
This comment has been minimized.
| bytes_deposit, | ||
| creation_time_usecs, | ||
| } = inner; | ||
| if bytes_deposit == 0 { |
There was a problem hiding this comment.
it's fine here, but for the future, if you add V2, you might want to actually have an enum in memory?
or you intentionally want to convert into newest possible?
There was a problem hiding this comment.
intentionally, because I feel better doing Eq when there's not two vairants representing the same thing.
|
and you'll need to regenerate all golden files after rebase :) |
Remove StateValueMetadataKind=Option<StateValueMetadata>, instead, add StateValueMetadata::is_none() as an indication of a None metadata. This way, we are able to upgrade the metadata format at runtime (when we charge and refund the storage fee). This is to prepare for the refundable permanent bytes.
msmouse
force-pushed
the
1211-alden-metadata-upgrade
branch
from
76d2d3d
to
f4b8fc2
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
❌ Forge suite
|
| @@ -193,6 +197,7 @@ pub struct WriteWithDelayedFieldsOp { | |||
| pub struct InPlaceDelayedFieldChangeOp { | |||
| pub layout: Arc<MoveTypeLayout>, | |||
| pub materialized_size: u64, | |||
| pub metadata: StateValueMetadata, | |||
There was a problem hiding this comment.
Sorry I missed this change - why do InPlaceDelayedFieldOps now carry metadata? @msmouse @igor-aptos
There was a problem hiding this comment.
if the size changes, we need to change the deposit
There was a problem hiding this comment.
this is not yet implemented, right? the reason I don't follow having metadata here is because there might be multiple Ops inside the same resource, so which one carries the right metadata? Is the metadata going to be overwritten in the final write-set?
if it is just for validation, then I thought the plan was to just validate the size that was used to compute the deposit and/or any charges?
There was a problem hiding this comment.
you mean multiple ops that get squashed?
each session starts from the output of the previous, so:
we start with "storage_size", and that is deposit in the metadata
then first session modifies size to "size_1", we charge for "size_1 - storage_size", and metadata is updated to size_1
then second session modifies to "size_2", we charge for "size_2 - size_1", and metadata is updated to size_2
squashing just needs to take the most recent metadata?
There was a problem hiding this comment.
sorry, I was confused about the struct, mis-remembered how it's used.
as per our discussion though, we should probably make the handling uniform now with at least ResourceGroupInPlaceDelayedFieldChangeOp and possibly even GroupWrites (and maybe avoid change to modification thingy we are doing there)
Description
Remove
StateValueMetadataKind=Option<StateValueMetadata>, instead, addStateValueMetadata::is_none()as a in memory representation of aNonemetadata. This way, we are able to upgrade the metadata format at runtime (when we charge and refund the storage fee). This is to prepare for the refundable permanent bytes.Magic is put in place for
StateValueandWriteOp, so they always carry a metadata in the latest format and serialize into the old formats if necessary.Test Plan