Skip to content
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.

Sign up for GitHub

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

Jump to bottom

websocket: send/receive reducer & table ids instead of names (take 2) #1940

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

websocket: send/receive reducer & table ids instead of names (take 2) #1940

wants to merge 2 commits into from

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Nov 5, 2024 •
edited
Loading

Description of Changes

Receive and send reducer ids and table ids as opposed to names in the SDK.

Alternative to #1883 which is more efficient, but also uses indices internally rather than strings, making the diff larger.

API and ABI breaking changes

Yes.

@Centril Centril changed the title Centril/name keyed websocket use ids websocket: send/receive reducer & table ids instead of names (take 2) Nov 5, 2024
@Centril Centril force-pushed the centril/name-keyed-websocket-use-ids branch from 08ed8fe to 273d4f1 Compare November 5, 2024 16:28
@Centril Centril marked this pull request as ready for review November 5, 2024 18:43
gefjon
gefjon reviewed Nov 5, 2024
View reviewed changes
Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for splitting this out. It's a much easier review this way. Notably, I was now able to find an actual semantic concern within this smaller diff. It should be easy to address.

bfops
bfops approved these changes Nov 8, 2024
View reviewed changes
Copy link
Collaborator

@bfops bfops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLI changes in generate/mod.rs and subscribe.rs LGTM

@bfops bfops linked an issue Nov 11, 2024 that may be closed by this pull request
Websocket API: Establish reducer/table ids <-> names during handshake (IdentityConnected) #1796
Open
cloutiertyler
cloutiertyler previously requested changes Nov 11, 2024
View reviewed changes
Copy link
Contributor

@cloutiertyler cloutiertyler left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requires a corresponding internal change to address the Internal Test failure.

@Centril Centril mentioned this pull request Nov 12, 2024
Open
1 task
@Centril Centril force-pushed the centril/name-keyed-websocket-use-ids branch from fc82682 to 5c1e4bd Compare November 12, 2024 15:14
@Centril Centril mentioned this pull request Nov 12, 2024
Open
@Centril Centril force-pushed the centril/name-keyed-websocket-use-ids branch 2 times, most recently from 5a753ee to 868d765 Compare November 18, 2024 18:14
@Centril Centril force-pushed the centril/name-keyed-websocket-use-ids branch from 868d765 to 40f4a91 Compare November 19, 2024 18:05
@Centril
websocket: send/receive reducer & table ids instead of names
4bfe615 
implement ids-no-names in c# sdk codegen

sdk: use strings instead of indices; less efficient, but smaller diff

sdk: check that expected reducers/tables form subset of actual
@Centril Centril force-pushed the centril/name-keyed-websocket-use-ids branch from 40f4a91 to 4bfe615 Compare November 19, 2024 18:08
@gefjon gefjon mentioned this pull request Nov 21, 2024
Closed
@gefjon gefjon assigned gefjon and unassigned gefjon Nov 25, 2024
@bfops
Copy link
Collaborator

bfops commented Dec 2, 2024

@Centril is this both API and ABI breaking? Can you add the appropriate labels?

@bfops bfops added api-break A PR that makes an API breaking change release-1.0 release-any To be landed in any release window and removed release-rc1-nice-to-have labels Dec 30, 2024
@cloutiertyler cloutiertyler removed release-1.0 release-any To be landed in any release window labels Feb 8, 2025
@bfops bfops marked this pull request as draft February 18, 2025 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bfops bfops bfops approved these changes

@gefjon gefjon gefjon approved these changes

@cloutiertyler cloutiertyler cloutiertyler left review comments

@jdetter jdetter Awaiting requested review from jdetter jdetter will be requested when the pull request is marked ready for review jdetter is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
api-break A PR that makes an API breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Websocket API: Establish reducer/table ids <-> names during handshake (IdentityConnected)
4 participants
@Centril @bfops @cloutiertyler @gefjon