-
Notifications
You must be signed in to change notification settings - Fork 177
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
Rename Address
to ConnectionId
#2220
Conversation
also: - removed `impl default for connectionid`, i want to be explicit about using zero. - renamed or re-documented many uses of the word "address" which were actually referring to the database identity, despite the concept of the database identity not having changed in this PR. - removed client address query param from call HTTP API - added warn log to connection id param in subscribe HTTP API so users don't do it - removed `client_address` from `MetadataFile`. It seemed to be unused, and I have no clue what it would possibly be for. - rust SDK is still supplying connection id to subscribe. this needs to change, but inconvenient. - C# `CallerIdentity` renamed to `Sender`, per tyler's request Expected to be in a very broken state right now, but I'm stepping away for a moment and wanted to save what I had so far.
Incl. two files that were not reachable and seemed severely bit-rotted: - `tracelog.rs` - `reducer.rs` The latter I myself forgot to delete in a past PR; the former I do not know about, but it's not part of our project, it wouldn't compile, and I don't see any reason to keep it in our repo.
Address
to ConnectionId
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.
My codeowned files LGTM 👍
crates/cli/src/subcommands/call.rs
crates/cli/src/subcommands/subscribe.rs
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.
Approving websocket.rs changes
…ss-to-connection-id
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 looked through all but the generated files, and all of the changes seemed right to me.
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.
Reviewed the files I'm a code owner of. Left just one suggested change.
…ss-to-connection-id
Co-authored-by: Tyler Cloutier <[email protected]> Signed-off-by: Phoebe Goldman <[email protected]>
…lockworklabs/SpacetimeDB into phoebe/rename-address-to-connection-id
## Description of Changes Companion to [Rename `Address` to `ConnectionId`](clockworklabs/SpacetimeDB#2220). See that PR's description for more. Like all the SDKs, this PR does not change the SDK's behavior; the SDK still generates a connection ID locally and passes it through the HTTP API. This is not exposed to users, and can be changed in a follow-up. Also, use `/usr/bin/env bash` instead of `/bin/bash` in tools, for portability reasons. ## API - [x] This is an API breaking change to the SDK `Address` is renamed to `ConnectionId`. ## Requires SpacetimeDB PRs - clockworklabs/SpacetimeDB#2220 - ## Testsuite *If you would like to run the your SDK changes in this PR against a specific SpacetimeDB branch, specify that here. This can be a branch name or a link to a PR.* SpacetimeDB branch name: phoebe/rename-address-to-connection-id ## Testing - [x] Pretty much just automated testing. - [x] @kazimuth will need to update and run Blackholio. --------- Co-authored-by: James Gilles <[email protected]>
Description of Changes
This PR renames
Address
, which was used only for identifying client connections at this point, toConnectionId
. Semantic changes are kept to a minimum.Notable features of this PR:
Address::from_byte_array
andAddress::to_byte_array
toConnectionId::from_le_byte_array
andConnectionId::to_le_byte_array
. My theory is that the name is changing anyways, so I might as well make the endianness explicit. This is a direct response to the comments all overaddress.rs
which talk about correct endianness, often IN ALL CAPS./database/subscribe
HTTP endpoint, which is the one that establishes a WS connection. However, I removed it from/database/call
.debug
level, when that query parameter is supplied.Companion PRs
API and ABI breaking changes
Yep! A significant, though superficial, API break.
Expected complexity level and risk
5: the word "address" was in just about all of our APIs, both internal and external, and they're all changing. Every single component we have needs to be updated in lockstep to account for this.
Testing
Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!