Introduce StorageCodec which helps with encoding/decoding of (codec, value) pairs #1389

tillrohrmann · 2024-04-11T21:09:03Z

Attempt to generalize the serialization of versioned values by introducing the VersionedSerializer trait. The versioned serializer is aware of the serialization format version and always writes it first (u16) before writing the actual value. When deserializing, the serializer first reads the version and then gives it to the deserialize method for the actual value. Thereby, the deserialization step can be conditioned on the actual version. One thing that becomes apparent is that by decoupling the domain object from the storage object, one needs to provide at the storage layer boundary information about which serializer to use for a given domain object.

Happy to hear what you think about this approach @AhmedSoliman.

tillrohrmann · 2024-04-12T12:14:58Z

Some open questions:

How fine-grained do we want to version the data?
- We could have a version for PP state, logs records and every metadata entity (as it is in this PR)
- We could bundle the metadata entities into a single version meaning that the overall version needs to be bumped if any entity changes
- Having a single storage version (probably not very practical)
How to couple the cluster marker version to the individual storage versions? Should the storage format versions be combined into the cluster marker version or be recorded separately? Probably just recording the actual Restate version is fine.

tillrohrmann · 2024-04-15T19:20:22Z

I've pushed an update to this PR which reflects our offline discussion @AhmedSoliman. The VersionedSerializer has now been replaced with StorageSerde which encodes for a given type how it is stored in storage (including its codec). PTAL.

Making the domain object aware of its storage type requires that we co-locate both. This is currently not the case for the pp storage objects where we have the storage-api crate and the storage-proto crate. Merging those two crates, I will do as a follow-up. I am a bit unhappy about this coupling but there is currently no clear value for keeping these crates separate other than from a purist point of view.

crates/storage-proto/src/lib.rs

crates/storage-rocksdb/src/codec.rs

crates/types/src/storage.rs

crates/wal-protocol/src/lib.rs

AhmedSoliman

Absolute legend! Thanks for bearing with me on this one.

AhmedSoliman · 2024-04-17T07:44:46Z

crates/core/src/metadata_store/mod.rs

-        // todo add proper format version
-        let value = bincode::serde::encode_to_vec(value, bincode::config::standard())
-            .map_err(|err| WriteError::Codec(err.into()))?;
+        let mut buf = BytesMut::default();


Potential future improvement would be to follow a design similar to storage-rocksdb's codec, if types can return an estimate of the buffer size needed for serialization, the buffer can be created with enough capacity to reduce the number of allocations.

Yes, indeed. This has become a bit harder now because before we already had a Protobuf type which easily gave us this information. Right now we only create the protobuf types for the protobuf-encoded values when actually serializing.

AhmedSoliman · 2024-04-17T07:49:12Z

crates/schema/src/lib.rs

 #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)]
 pub struct Schema {
    pub version: Version,
    pub components: HashMap<String, ComponentSchemas>,
+    // flexbuffers only supports string-keyed maps :-( --> so we store it as vector of kv pairs
+    #[serde_as(as = "serde_with::Seq<(_, _)>")]
    pub deployments: HashMap<DeploymentId, DeploymentSchemas>,


What would happen if one forgot to add the serde_as here?

It would fail at runtime with a KeyMustBeString error if the map contained an entry.

AhmedSoliman · 2024-04-17T07:49:53Z

crates/schema/src/lib.rs

+    #[serde_as(as = "serde_with::Seq<(_, _)>")]
    pub subscriptions: HashMap<SubscriptionId, Subscription>,


potential future improvement would be to wrap serializable maps into a new type to DRY.

Yes, this makes sense. Good idea.

AhmedSoliman · 2024-04-17T07:51:17Z

crates/schema/src/lib.rs

+    use crate::Schema;
+    use restate_types::flexbuffers_storage_encode_decode;
+
+    flexbuffers_storage_encode_decode!(Schema);


turning this into a proc-macro (derive) would be a nice exercise :)

Indeed. I didn't want to press my luck with macros yesterday ;-)

AhmedSoliman · 2024-04-17T08:07:26Z

crates/types/src/storage.rs

+/// To support codec evolution, this trait implementation needs to be able to decode values encoded
+/// with any previously used codec.
+pub trait StorageDecode {
+    fn decode(buf: &[u8], kind: StorageCodecKind) -> Result<Self, StorageDecodeError>


Potential future improvement might to change the input to &mut T where T: Buf to make it easier to decode from cursors (and to advance it for incremental decoding).

True. Let me check whether I can quickly change it as part of this PR.

Making the storage-api crate aware of the storage types will allow us to implement for each API type how it will be serialized. This increases coupling but simplifies things until we truely need to have multiple storage representations.

StorageSerde encodes how a given type is encoded for storing in storage.

We move the dedup types into the storage-api crate so that we can implement StorageSerde for them based on the protobuf types defined in this crate.

This commit removes the partial storage of the VirtualObjectStatus to simplify transition to the StorageSerde. Optimizations for what to not store can be done later.

We now require that stored values implement the StorageEncode trait to encode them together with their codec. When reading a value, the value needs to implement StorageDecode. Alternatively, one can also store raw bytes into RocksDB as it is needed for state values, for example.

Flexbuffers does not support non-string keyed maps. Therefore, we have to serialize maps that have a non-string key as a vector of key-value pairs.

tillrohrmann · 2024-04-17T13:35:40Z

Thanks for all your valuable feedback @AhmedSoliman 🙏 Merging this PR once GHA gives green light.

tillrohrmann requested a review from AhmedSoliman April 11, 2024 21:09

tillrohrmann force-pushed the serialization branch from 5ea54a8 to cd5229e Compare April 16, 2024 07:36

AhmedSoliman reviewed Apr 16, 2024

View reviewed changes

tillrohrmann force-pushed the serialization branch from cd5229e to 5416f15 Compare April 16, 2024 15:00

tillrohrmann changed the title ~~Introduce VersionedSerializer which helps with serde of versioned values~~ Introduce StorageCodec which helps with encoding/decoding of (codec, values) pairs Apr 16, 2024

tillrohrmann marked this pull request as ready for review April 16, 2024 16:46

tillrohrmann requested a review from AhmedSoliman April 16, 2024 16:46

tillrohrmann changed the title ~~Introduce StorageCodec which helps with encoding/decoding of (codec, values) pairs~~ Introduce StorageCodec which helps with encoding/decoding of (codec, value) pairs Apr 16, 2024

tillrohrmann force-pushed the serialization branch 3 times, most recently from 382d9ea to a8a2b31 Compare April 16, 2024 17:11

AhmedSoliman approved these changes Apr 17, 2024

View reviewed changes

tillrohrmann mentioned this pull request Apr 17, 2024

Introduce KeyPrefix for RocksDB table keys #1416

Closed

tillrohrmann added 7 commits April 17, 2024 14:33

Introduce StorageSerde

3a543b9

StorageSerde encodes how a given type is encoded for storing in storage.

Move dedup types into storage-api crate

5c67021

We move the dedup types into the storage-api crate so that we can implement StorageSerde for them based on the protobuf types defined in this crate.

Don't be smart about how to store the VirtualObjectStatus

b4700ff

This commit removes the partial storage of the VirtualObjectStatus to simplify transition to the StorageSerde. Optimizations for what to not store can be done later.

Store protobuf codec with partition processor values

85937b0

Store non-string keyed maps as vectors of key-value pairs

59549fe

Flexbuffers does not support non-string keyed maps. Therefore, we have to serialize maps that have a non-string key as a vector of key-value pairs.

tillrohrmann force-pushed the serialization branch from a8a2b31 to 6e74ed0 Compare April 17, 2024 13:34

Change StorageDecode::decode to accept Buf implementations

d32f3ef

tillrohrmann force-pushed the serialization branch from 6e74ed0 to d32f3ef Compare April 17, 2024 13:35

tillrohrmann merged commit d32f3ef into restatedev:main Apr 17, 2024
6 checks passed

tillrohrmann deleted the serialization branch April 17, 2024 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce StorageCodec which helps with encoding/decoding of (codec, value) pairs #1389

Introduce StorageCodec which helps with encoding/decoding of (codec, value) pairs #1389

tillrohrmann commented Apr 11, 2024

tillrohrmann commented Apr 12, 2024 •

edited

Loading

tillrohrmann commented Apr 15, 2024

AhmedSoliman left a comment

AhmedSoliman Apr 17, 2024

tillrohrmann Apr 17, 2024 •

edited

Loading

tillrohrmann Apr 17, 2024

AhmedSoliman Apr 17, 2024

tillrohrmann Apr 17, 2024

AhmedSoliman Apr 17, 2024

tillrohrmann Apr 17, 2024

tillrohrmann Apr 17, 2024

AhmedSoliman Apr 17, 2024

tillrohrmann Apr 17, 2024

tillrohrmann Apr 17, 2024

AhmedSoliman Apr 17, 2024

tillrohrmann Apr 17, 2024

tillrohrmann commented Apr 17, 2024

		#[serde_as(as = "serde_with::Seq<(_, _)>")]
		pub subscriptions: HashMap<SubscriptionId, Subscription>,

Introduce StorageCodec which helps with encoding/decoding of (codec, value) pairs #1389

Introduce StorageCodec which helps with encoding/decoding of (codec, value) pairs #1389

Conversation

tillrohrmann commented Apr 11, 2024

tillrohrmann commented Apr 12, 2024 • edited Loading

tillrohrmann commented Apr 15, 2024

AhmedSoliman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tillrohrmann Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tillrohrmann commented Apr 17, 2024

tillrohrmann commented Apr 12, 2024 •

edited

Loading

tillrohrmann Apr 17, 2024 •

edited

Loading