-
Notifications
You must be signed in to change notification settings - Fork 668
Split WS into v1 and v2; make various changes in v2. #4023
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: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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