Split WS into v1 and v2; make various changes in v2.#4023
Split WS into v1 and v2; make various changes in v2.#4023
Conversation
We're still supporting v1, so we can't really nix it from the codebase. Is there still a reason to remove it from v2? |
Unless you want to eventually support per-client energy limits/balances! |
Unless you also remove (or add a setting to disable) WS v1, this won't be really helpful for security. |
@joshua-spacetime Each additional supported format increases the worst-case amount of serialization we have to do at runtime, and the volume of serialization code we have to write and maintain. The V2 definitions without switching on Also, forcing clients to migrate to BSATN in order to use the new features introduced by V2 gives us a path to eventually deprecating JSON over WebSockets entirely. I imagine we will someday deprecate and remove the V1 format, though it will probably not be for quite some time.
@egormanga We have no plans to do this within the expected lifetime of the V2 WebSocket format. If that changes and we do, clients could still receive an
@egormanga Just because we can't immediately remove the old format doesn't mean we'll be stuck with it forever. Gradual deprecation and eventual removal is a graceful path which still allows us to improve SpacetimeDB. |
|
This looks good to me. I'm all for removing |
| pub enum ReducerOutcome { | ||
| Ok(SubscriptionOk), | ||
| Err(Bytes), | ||
| InternalError(Box<str>), |
There was a problem hiding this comment.
Alt: We could have the internal error be an enum type so that we can change this type later if we want a more structure error (e.g. status codes or something). Not critical.
About this, I ran into a few issues with the JSON format and the best way to go forward with the website (specifically the table explorer) is to switch to BSATN, especially if we want to properly show u64 values (currently they aren't displayed properly if they go above 2^53. |
| pub struct PersistentTableRows { | ||
| pub inserts: Box<[Bytes]>, | ||
| pub deletes: Box<[Bytes]>, |
There was a problem hiding this comment.
Additionally I think we should also add support for row diffs in the new format.
There was a problem hiding this comment.
I've added the following comment to TableUpdateRows:
/// The rows of a [`TableUpdate`], separated based on the kind of table.
///
/// Regular "persistent" tables will include a list of inserted rows and a list of deleted rows.
/// Event tables, whose rows are not persistent, will instead include a single list of event rows.
///
/// In the future, we may add additional variants to this enum.
/// In particular, we may add a variant for in-place updates of rows for tables with primary keys.
/// Note that clients will need to opt in to using this new variant,
/// to preserve compatibility of clients which predate the new variant.
#[derive(SpacetimeType)]
#[sats(crate = spacetimedb_lib)]
pub enum TableUpdateRows {
PersistentTable(PersistentTableRows),
EventTable(EventTableRows),
}Incl. mention of future possibility of PK-ful partial updates.
# Description of Changes This adds the v2 websocket protocol and adds support on the server side. For context on many of the changes/decisions, you can look at the discussion on #4023. To restate some of the key changes: - The reducer event information is no longer sent with transaction updates (because we don't want to broadcast reducer call information anymore). - If a client calls a reducer, they are sent a `ReducerResult` which includes the outcome of the reducer call and and related row updates for queries that the client is subscribed to. - We no longer dedupe queries that appear in multiple query sets for the same client. This is because we are moving toward per-query storage. - Related to that, Unsubscribe requests have an option to send the related rows. We need this for now, since clients don't have per-query storage implemented yet. - We don't have the json format in v2. Notes for reviewers: - This moves around the messages in `crates/client-api-messages/src/websocket` (into `common`, `v1`, and `v2`), and this renaming of existing messages adds a lot of noise to the PR. - In many places, I chose to duplicate a lot of code to have a v1 version and a v2 version. I went with this to make it easier to remove the v1 version in the future (hopefully we can just fully delete most of the v1 functions). - `module_subscription_manager.rs` has probably has the biggest changes, since we now track queries by query_set_id, and we get to remove some complexity of v1's FormatSwitch. <!-- Please describe your change, mention any related tickets, and so on here. --> # API and ABI breaking changes The v1 protocol still works, though we won't send the reducer event info for v10 modules. # Expected complexity level and risk 4. This touches a lot of places. # Testing Unit testing is pretty minimal for the new code paths. I've done some manual e2e testing with the typescript quickstart, and this has been tested with a different branch implementing the v2 rust client. --------- Co-authored-by: Phoebe Goldman <phoebe@goldman-tribe.org> Co-authored-by: Jeffrey Dallatezza <jeffreydallatezza@gmail.com>
Description of Changes
Draft PR, currently containing only the new format itself.
The changes in question are:
Remove
total_host_execution_duration_microsTransactionUpdatemessages, to show its latency overlay.Add query IDs to database updates
Remove rows from
UnsubscribeAppliedUnsubscribeAppliedas a confirmation, though this is unnecessary and I may change my mind.Remove
table_idfromSubscriptionErrorRename
QueryIdtoQuerySetIdRemove
OutOfEnergyerror variantsInternalError.Remove
ReducerEventinfo fromTransactionUpdateSeparate
ReducerResultfromTransactionUpdateReducerResultwraps aTransactionUpdatewhen successful.ReducerResultalso gets a return value when Ok.Nix
FormatSwitchand the JSON formatRemove table IDs
Open questions:
Is
CallReducerFlags::NoSuccessNotifystill useful?An empty
ReducerResultshould be much smaller now than a v1 emptyTransactionUpdate, since it doesn't need to contain the arguments and such. By my count it would be 17 bytes (not counting whatever WS framing):API and ABI breaking changes
Expected complexity level and risk
Testing