LogServer Bilrost (v2) message types #3176

muhamadazmy · 2025-04-17T17:17:13Z

LogServer Bilrost (v2) message types

Move log server messages to Bilrost v2 by either derive bilrost::Message
or implement a DTO

Stack created with Sapling. Best reviewed with ReviewStack.

muhamadazmy · 2025-04-18T10:22:05Z

crates/types/src/net/log_server.rs

@@ -331,12 +417,29 @@ impl Released {
    }
 }

+impl V2Convertible for Released {


NOTICE:

The Released message (and other response messages) carries only a header. This allows us to convert this message (and similar ones) directly to message::LogServerResponseHeader over the wire so we don't have to create a shadow messages for this as well.

But this comes with a cavity. We can't modify this type later to add more fields to Released without breaking compatibility. The only workaround in that case is to
flatten header directly inside Release. Then add the extra field.

To avoid this situation we can enforce the following conventions:

Only use this pattern if you are 100% sure this message will not change in the future (Will Released ever has new fields). In that case create separate message type.

Create a GenericResponse type, that only has the header field. Then use it as Target in the V2Convertible implementation for all messages that matches. Now if we need to add more fields to Released it then can get its own shadow type without breaking compatibility.

I've removed Released in my changes, it's not used and not needed.

This is not about the Release message itself but rather how we serialize it (or similar messages over the wire)

AhmedSoliman

Thanks @mohammedazmy. I've left some notes for your consideration.

crates/types/derive/src/flatten.rs

crates/types/protobuf/restate/common.proto

crates/types/src/dto.rs

crates/types/src/logs/record.rs

crates/types/src/net/codec.rs

crates/types/src/net/mod.rs

crates/types/src/logs/mod.rs

crates/types/src/net/log_server.rs

muhamadazmy · 2025-04-30T11:38:45Z

@AhmedSoliman This PR has improved a lot (IMHO) after using the BilrostAs macro. It minimized the number of structs that needed a dto. I would love to get your feedback specially around the design decision that I am not 100% sure about, like introducing some unknown variants only for the type default state required by Bilrost empty state and how to handle them.

AhmedSoliman · 2025-05-02T12:14:13Z

crates/log-server/src/loglet_worker.rs

+                TailUpdateQuery::Unknown => {
+                    unreachable!()
+                }


Wouldn't it be better to fail the operation and return TailUpdated with_status(Status::Malformed)?

Oh, definitely! Sorry I didn't see it.

AhmedSoliman · 2025-05-02T12:22:21Z

crates/types/src/logs/mod.rs

+
+    impl From<&super::KeyFilter> for KeyFilter {
+        fn from(value: &crate::logs::KeyFilter) -> Self {
+            use crate::logs::KeyFilter::*;


I wouldn't recommend this unless the number of variants is (a) too large and (b) doesn't include names that might conflict with std lib's prelude types. This case doesn't meet (a) and it's borderline close to hitting (b) due to rust's Any type, albeit it's not in prelude.

AhmedSoliman · 2025-05-02T12:23:43Z

crates/types/src/net/log_server.rs

+    NetSerde,
+)]
+#[bilrost_as(dto::Payloads)]
+pub struct Payloads(Arc<[Record]>);


Does this break compatibility with V1?

No it doesn't. I already have tested it also to be sure against a V1 node. Serde serializes newtypes as it's inner type.

AhmedSoliman · 2025-05-06T10:36:35Z

crates/types/src/net/log_server.rs

@@ -181,7 +204,7 @@ define_logserver_rpc! {

 // WaitForTail
 define_logserver_rpc! {
-    @request = WaitForTail,
+    @request = WaitForTail ,


Suggested change

@request = WaitForTail ,

@request = WaitForTail,

Move log server messages to Bilrost v2 by either derive bilrost::Message or implement a DTO

This was referenced Apr 17, 2025

Add support for protocol version Bilrost (v2) #3175

Closed

Using Bilrost for GetNodeStatus messages #3172

Merged

muhamadazmy force-pushed the pr3176 branch 2 times, most recently from 85a78f1 to 2c0c399 Compare April 18, 2025 10:05

muhamadazmy commented Apr 18, 2025

View reviewed changes

muhamadazmy force-pushed the pr3176 branch from 2c0c399 to 7884c14 Compare April 21, 2025 13:31

muhamadazmy requested review from tillrohrmann and AhmedSoliman April 22, 2025 06:44

muhamadazmy force-pushed the pr3176 branch 2 times, most recently from 9901d77 to cccbad3 Compare April 23, 2025 11:14

muhamadazmy mentioned this pull request Apr 23, 2025

Add support for protocol version Bilrost (v2) #3190

Merged

muhamadazmy force-pushed the pr3176 branch 4 times, most recently from 9893a58 to 40a4ad9 Compare April 25, 2025 10:38

muhamadazmy changed the title ~~WIP: LogServer v2 message types~~ LogServer Bilrost (v2) message types Apr 25, 2025

muhamadazmy marked this pull request as ready for review April 25, 2025 10:38

muhamadazmy force-pushed the pr3176 branch from 40a4ad9 to a4dc306 Compare April 25, 2025 10:51

AhmedSoliman requested changes Apr 25, 2025

View reviewed changes

muhamadazmy force-pushed the pr3176 branch 11 times, most recently from da3a2e9 to 88263f0 Compare April 28, 2025 09:47

muhamadazmy force-pushed the pr3176 branch 3 times, most recently from 5cbb765 to 8015d48 Compare April 29, 2025 07:10

AhmedSoliman reviewed Apr 29, 2025

View reviewed changes

crates/types/src/logs/mod.rs Show resolved Hide resolved

AhmedSoliman requested changes Apr 29, 2025

View reviewed changes

muhamadazmy force-pushed the pr3176 branch 2 times, most recently from 628666e to 512c087 Compare April 30, 2025 11:07

muhamadazmy mentioned this pull request Apr 30, 2025

introduce BilrostAs derive macro #3208

Merged

muhamadazmy force-pushed the pr3176 branch 4 times, most recently from 30639fa to 306fd90 Compare April 30, 2025 11:32

muhamadazmy requested a review from AhmedSoliman April 30, 2025 11:34

muhamadazmy force-pushed the pr3176 branch from 306fd90 to dc58f55 Compare April 30, 2025 14:24

muhamadazmy mentioned this pull request Apr 30, 2025

Bilrost Metadata messages #3212

Closed

muhamadazmy force-pushed the pr3176 branch 3 times, most recently from af79d28 to 0752b5a Compare May 2, 2025 08:54

AhmedSoliman reviewed May 2, 2025

View reviewed changes

muhamadazmy force-pushed the pr3176 branch 2 times, most recently from 45fb08d to ca902b3 Compare May 5, 2025 14:53

muhamadazmy mentioned this pull request May 5, 2025

Bilrost fast encoding + more common helper types #3222

Merged

muhamadazmy requested a review from AhmedSoliman May 6, 2025 08:33

AhmedSoliman approved these changes May 6, 2025

View reviewed changes

LogServer Bilrost (v2) message types

64aa9bc

Move log server messages to Bilrost v2 by either derive bilrost::Message or implement a DTO

muhamadazmy force-pushed the pr3176 branch from ca902b3 to 64aa9bc Compare May 6, 2025 11:10

muhamadazmy merged commit fa6aa2f into restatedev:main May 6, 2025
58 of 60 checks passed

muhamadazmy deleted the pr3176 branch May 6, 2025 12:06

github-actions bot locked and limited conversation to collaborators May 6, 2025

LogServer Bilrost (v2) message types #3176

LogServer Bilrost (v2) message types #3176

Uh oh!

Conversation

muhamadazmy commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AhmedSoliman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

muhamadazmy commented Apr 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

muhamadazmy commented Apr 17, 2025 •

edited

Loading