-
Notifications
You must be signed in to change notification settings - Fork 118
[storage] Envelope v2 #3853
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
base: main
Are you sure you want to change the base?
[storage] Envelope v2 #3853
Conversation
75eea1e to
7464a02
Compare
|
@tillrohrmann in case you want to take a look the interesting parts are in the v2.rs file in this PR |
crates/types/src/message.rs
Outdated
| } | ||
|
|
||
| #[derive(Debug, Clone, Copy, bilrost::Message)] | ||
| pub struct MessageIndexRecrod { |
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.
typo: Record.
crates/types/src/message.rs
Outdated
| pub index: MessageIndex, | ||
| } | ||
|
|
||
| bilrost_storage_encode_decode!(MessageIndexRecrod); |
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.
Perhaps the type should be named after the wal command (i.e. TruncateOutboxRequest) which btw needs to contain the PartitionId.
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.
Agree! As for the partition-id this record message was mapped 1:1 from v1 without much thought about the payload it carries. I think the reason it doesn't have the partition id is that that's part of the envelope Header (destination)
crates/wal-protocol/src/v2.rs
Outdated
| #[bilrost(3)] | ||
| dedup: Dedup, | ||
| #[bilrost(4)] | ||
| keys: Keys, |
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.
I wonder if this is actually needed. On the read path, we ship the keys directly from bifrost (one layer up from the envelope). On the write-path, the appender needs the input record to implement HasRecordKeys which could be implemented on a wrapper type used as input to the appender. Essentially making the input type different than output type to save a few bytes on each record.
crates/wal-protocol/src/v2.rs
Outdated
| impl WithPartitionKey for Header { | ||
| fn partition_key(&self) -> PartitionKey { | ||
| match self.destination { | ||
| Destination::None => unimplemented!("expect destinationt to be set"), |
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.
typo @ destinationt
crates/wal-protocol/src/v2.rs
Outdated
| #[bilrost(1)] | ||
| source: Source, | ||
| #[bilrost(2)] | ||
| destination: Destination, |
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.
destination is potentially unnecessary unless you see a clear use-case for it.
crates/wal-protocol/src/v2.rs
Outdated
| { | ||
| match kind { | ||
| StorageCodecKind::FlexbuffersSerde => { | ||
| todo!("implement loading from envelop V1") |
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.
| todo!("implement loading from envelop V1") | |
| todo!("implement loading from envelope V1") |
crates/wal-protocol/src/v2.rs
Outdated
| record! { | ||
| @name=PurgeJournal, | ||
| @kind=RecordKind::PurgeJournal, | ||
| @payload=PurgeInvocationRequest |
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.
is it true that it has the exact same payload as PurgeInvocation?
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.
That's what v1 does. Yes.
60e7920 to
2b53068
Compare
505bbf7 to
8e7cd15
Compare
c7dba21 to
fc48bd2
Compare
93592e3 to
a5eea7e
Compare
# Summary - Separate header, keys, and other envelope related information from the actual payload of the envelope - Ability to decode envelope header without decoding the full envelope - Support different encoding for the payload (records) (flexbuffers, or bilrost) - Delay decoding of payload until needed # TODO: - [x] Decode v1 envelope into v2 - [ ] Use Envelope v2 in code - [ ] Convert from v2 to v1 (to write)
| /// and serialization details required to interpret the payload. | ||
| #[derive(Debug, Clone, bilrost::Message)] | ||
| pub struct Header { | ||
| #[bilrost(1)] |
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.
Any particular reason why those tags are not 1,2,3?
| where | ||
| Self: Sized, | ||
| { | ||
| debug_assert_eq!(kind, $crate::storage::StorageCodecKind::Bilrost); |
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.
nit: I'd suggest we make this an assert_eq
[storage] Envelope v2
Summary
information from the actual payload of the envelope
TODO:
Stack created with Sapling. Best reviewed with ReviewStack.
Envelopev2 #4058