Protobufectomy #1077

coolreader18 · 2024-04-11T02:37:48Z

Description of Changes

Replace protobuf with bsatn.

API and ABI breaking changes

API breaking for binary clients.

Expected complexity level and risk

3 (lotsa churn)

Testing

This is dependent on the C# sdk getting updated. I feel confident that CI + a botstest with the updated sdk would get everything.

gefjon

So far I've only reviewed changes to client-api-messages, the Rust SDK and its codegen. Those look excellent. I've left a few low-priority questions and doc-related suggestions.

gefjon · 2024-04-11T14:10:44Z

crates/client-api-messages/src/ws.rs

+/// It also will not check for duplicate IDs. They are just a way to match responses to messages.
+#[derive(Deserialize, Serialize, serde::Deserialize)]
+pub struct OneOffQuery {
+    pub message_id: Vec<u8>,


Why is this a Vec<u8> rather than a u32 or something?

It's like that on older proto schema, we have a ticket to fix this - #927 waiting for breaking release. May be, this is a correct time to do it.

Oh, that'd be great, I'd been wondering about that as well.

gefjon · 2024-04-11T14:10:57Z

crates/client-api-messages/src/ws.rs

+/// Query should be a "SELECT * FROM Table WHERE ...". Other types of queries will be rejected.
+/// Multiple such semicolon-delimited queries are allowed.
+///
+/// One-off queries are identified by a client-generated messageID.
+/// To avoid data leaks, the server will NOT cache responses to messages based on UUID!
+/// It also will not check for duplicate IDs. They are just a way to match responses to messages.


Doc comments should be moved to fields.

Ya - I tried to do that for the most part; most/all of the comments are copied from the protobuf, which tended to put field docs on the outer type.

gefjon · 2024-04-11T14:11:48Z

crates/client-api-messages/src/ws.rs

+pub struct Subscribe {
+    /// A sequence of SQL queries.
+    pub query_strings: Vec<String>,
+    pub request_id: u32,


Doc comment please.

gefjon · 2024-04-11T14:12:09Z

crates/client-api-messages/src/ws.rs

+    // /// A reducer specified by its numerical ID.
+    // Id(u32),


Why is this in here?

gefjon · 2024-04-11T14:12:23Z

crates/client-api-messages/src/ws.rs

+pub struct CallReducer<Args> {
+    /// The name or id of the reducer to call.
+    pub reducer: ReducerId,
+    /// The arguments to the reducer, encoded as ????.


gefjon · 2024-04-11T14:12:46Z

crates/client-api-messages/src/ws.rs

+use crate::timestamp::Timestamp;
+
+#[derive(Deserialize, Serialize, serde::Deserialize)]
+pub enum ClientMessage<Args = Row> {


Suggested change

pub enum ClientMessage<Args = Row> {

/// A message sent by the client to the server.

pub enum ClientMessage<Args = Row> {

gefjon · 2024-04-11T14:13:04Z

crates/client-api-messages/src/ws.rs

+}
+
+#[derive(Serialize, Deserialize, derive_more::From)]
+pub enum ServerMessage {


Suggested change

pub enum ServerMessage {

/// A message sent by the server to the client.

pub enum ServerMessage {

gefjon · 2024-04-11T14:18:14Z

crates/sdk/src/identity.rs

-        }
-    }
-}
+pub use spacetimedb_lib::Identity;


This closes an outstanding ticket I had. You're my hero.

Lol - it was breaking something with a type mismatch and I didn't wanna do a from_bytes(to_bytes()) thing to convert it so this seemed simple enough lol.

gefjon · 2024-04-11T14:42:25Z

Outstanding TODOs:

C# SDK.
Unity package of C# SDK.
Typescript SDK.
Tests of above.
Minor updates to Rust SDK docs now that Identity has been changed to use the actual spacetimedb_lib::Identity type.

(Plus make the tests and lints pass, obviously.)

kazimuth

This all seems straightforward enough, I don't have any particular commentary. Thanks for getting it in!

I wonder if I can auto-generate the C# side of this interface...

kazimuth · 2024-04-11T15:40:57Z

crates/bindings-macro/src/lib.rs

@@ -45,6 +45,9 @@ mod sym {
    /// Matches `sats`.
    pub const SATS: Symbol = Symbol("sats");

+    /// Matches `transparent`.


Is this used on stuff in this PR? Just curious

kazimuth

Additional question: how do we negotiate protocol version now? I believe protobuf transparently supports changing schemas for the most part by making everything optional, but I don't think bsatn has anything like that. We should at minimum send a version number I think, either once per session or once per message...

kazimuth · 2024-04-11T17:15:05Z

crates/client-api-messages/src/ws.rs

@@ -0,0 +1,266 @@
+use bytes::Bytes;


Would like a module doc comment, also a crate doc comment.

kazimuth · 2024-04-11T17:15:53Z

crates/client-api-messages/src/ws.rs

+use crate::timestamp::Timestamp;
+
+#[derive(Deserialize, Serialize, serde::Deserialize)]
+pub enum ClientMessage<Args = Row> {


Do we need to add additional messages here for protocol version negotiation?

Centril · 2024-04-11T17:05:39Z

crates/client-api-messages/src/ws.rs

+    pub inserts: Vec<Row>,
+}
+
+#[derive(Debug, Clone)]


Suggested change

#[derive(Debug, Clone)]

#[derive(Debug, Clone, From)]

Centril · 2024-04-11T17:05:53Z

crates/client-api-messages/src/ws.rs

+impl From<Bytes> for Row {
+    fn from(b: Bytes) -> Self {
+        Row(b)
+    }
+}


Suggested change

impl From<Bytes> for Row {

fn from(b: Bytes) -> Self {

Row(b)

}

}

Centril · 2024-04-11T17:06:18Z

crates/client-api-messages/src/lib.rs

 pub mod name;
 pub mod recovery;
+pub mod timestamp;
+pub mod ws;


Suggested change

pub mod ws;

pub mod web_socket;

Centril · 2024-04-11T17:12:57Z

crates/bindings-macro/src/module.rs

@@ -21,17 +21,28 @@ pub(crate) struct SatsType<'a> {
 pub(crate) enum SatsTypeData<'a> {
    Product(Vec<SatsField<'a>>),
    Sum(Vec<SatsVariant<'a>>),
+    Transparent(SatsField<'a>),


Could you add some docs here?

Centril · 2024-04-11T17:17:22Z

crates/core/src/host/module_host.rs

@@ -642,6 +618,11 @@ impl ModuleHost {
        &self.info.subscriptions
    }

+    /// internal method to access the DatabaseInstanceContext for the module. don't make this public.


Can this be reflected in the method name?

I'd like to request that the method becomes public API.

The lifetimes of dbic and module should become coupled anyway.

Centril · 2024-04-11T17:17:55Z

crates/core/src/messages/mod.rs

@@ -2,3 +2,6 @@ pub mod control_db;
 pub mod control_worker_api;
 pub mod instance_db_trace_log;
 pub mod worker_db;
+pub mod ws {


Suggested change

pub mod ws {

pub mod web_socket {

coolreader18 requested a review from cloutiertyler as a code owner April 11, 2024 02:37

coolreader18 force-pushed the noa/protobufectomy branch from efcd187 to 5d4080a Compare April 11, 2024 05:04

gefjon reviewed Apr 11, 2024

View reviewed changes

kazimuth reviewed Apr 11, 2024

View reviewed changes

Centril reviewed Apr 11, 2024

View reviewed changes

coolreader18 force-pushed the noa/protobufectomy branch 2 times, most recently from e55d78c to 4d5cdbc Compare April 12, 2024 20:03

bfops added abi-break A PR that makes an ABI breaking change release-1.0 labels Apr 15, 2024

kim mentioned this pull request Apr 17, 2024

core: Store inputs (reducer info + args) in commitlog #1091

Merged

1 task

coolreader18 force-pushed the noa/protobufectomy branch 3 times, most recently from 5e9d2e2 to f77bfac Compare April 17, 2024 19:46

This was referenced May 3, 2024

Subscriptions: Fuse BSATN for all table row ops in one allocation #1201

Open

[EXPERIMENT]: Subscriptions: Fuse BSATN for all table row ops in one allocation #1200

Draft

coolreader18 added 4 commits May 13, 2024 12:44

Process generics in derive macros

b3239ee

Remove protobuf in core

c1131b6

Remove protobuf in sdk crate

ea0b8db

Remove protobuf from dockerfiles

939f843

coolreader18 force-pushed the noa/protobufectomy branch from f77bfac to f111123 Compare May 13, 2024 17:07

Allow codegenning from ws api

14b19e0

coolreader18 force-pushed the noa/protobufectomy branch from f111123 to 14b19e0 Compare May 14, 2024 02:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protobufectomy #1077

Protobufectomy #1077

coolreader18 commented Apr 11, 2024

gefjon left a comment

gefjon Apr 11, 2024

Shubham8287 Apr 11, 2024

coolreader18 Apr 12, 2024

gefjon Apr 11, 2024

coolreader18 Apr 12, 2024

gefjon Apr 11, 2024

gefjon Apr 11, 2024

gefjon Apr 11, 2024

gefjon Apr 11, 2024

gefjon Apr 11, 2024

gefjon Apr 11, 2024

coolreader18 Apr 12, 2024

gefjon commented Apr 11, 2024 •

edited

kazimuth left a comment

kazimuth Apr 11, 2024

kazimuth left a comment

kazimuth Apr 11, 2024

kazimuth Apr 11, 2024 •

edited

Centril Apr 11, 2024

Centril Apr 11, 2024

Centril Apr 11, 2024

Centril Apr 11, 2024

Centril Apr 11, 2024

kim Apr 17, 2024

Centril Apr 11, 2024

	pub enum ClientMessage<Args = Row> {
	/// A message sent by the client to the server.
	pub enum ClientMessage<Args = Row> {

	pub enum ServerMessage {
	/// A message sent by the server to the client.
	pub enum ServerMessage {

Protobufectomy #1077

Are you sure you want to change the base?

Protobufectomy #1077

Conversation

coolreader18 commented Apr 11, 2024

Description of Changes

API and ABI breaking changes

Expected complexity level and risk

Testing

gefjon left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gefjon commented Apr 11, 2024 • edited

kazimuth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazimuth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazimuth Apr 11, 2024 • edited

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

gefjon commented Apr 11, 2024 •

edited

kazimuth Apr 11, 2024 •

edited