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.

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

feat!: server-driven unsubscribes #1240

Merged
merged 13 commits into from May 16, 2024
Merged

Conversation

icehaunter
Copy link
Contributor

  1. Adds new boundary messages to the protocol allowing the server to send a GONE batch - a set of messages for the client to know that those rows will not receive further updates after an unsubscribe call.
  2. Server is the source of truth on which rows the client should have
  3. Only while-online unsubscribes are possible
  4. Client may reconnect at any point after sending the unsubscribe call and the server will adjust seen rows correctly and send a GONE batch if we don't believe the client had seen that -- see tests in subscriptions_test.exs.

Missing is a public API for unsubscribes, that's in the next PR.

Copy link

linear bot commented May 8, 2024

Copy link
Contributor

@kevin-dp kevin-dp left a comment

Choose a reason for hiding this comment

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

Overall LGTM, left some comments here and there but nothing major.
Note: i reviewed the TS part and protocol, did not look into the server side.

clients/typescript/src/migrators/query-builder/builder.ts Outdated Show resolved Hide resolved
clients/typescript/src/migrators/query-builder/builder.ts Outdated Show resolved Hide resolved
clients/typescript/src/migrators/query-builder/builder.ts Outdated Show resolved Hide resolved
clients/typescript/src/migrators/query-builder/builder.ts Outdated Show resolved Hide resolved
clients/typescript/src/migrators/query-builder/builder.ts Outdated Show resolved Hide resolved
clients/typescript/src/satellite/client.ts Outdated Show resolved Hide resolved
clients/typescript/src/util/proto.ts Show resolved Hide resolved
clients/typescript/src/util/types.ts Show resolved Hide resolved
clients/typescript/test/satellite/process.ts Show resolved Hide resolved
Copy link
Contributor

@magnetised magnetised left a comment

Choose a reason for hiding this comment

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

excellent work. you do need to fix those protobuf id conflicts tho.

@icehaunter icehaunter requested a review from magnetised May 9, 2024 16:26
CREATE TABLE #{Extension.client_unsub_points_table()} (
client_id VARCHAR(64) NOT NULL,
subscription_id UUID NOT NULL,
wal_pos BIGINT NOT NULL,
Copy link
Member

Choose a reason for hiding this comment

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

I remember you raised an issue with this BIGINT type. It's not too late to change it here. Making the same change in the older migration can be done in a separate PR (I can handle that).

Enum.map(moved_in_records, fn {id, record} -> {id, %Changes.NewRecord{record: record}} end)
# We're converting these records to a list of keys to query next layers on and building a fake-rooted graph.
# We're "rooting" the top layer of records so that `SentRowsGraph` functions know where to start traversal.
# It's important to get of that fake root later.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what this says. "Get off that fake root"? At what point later?

Copy link
Contributor

@magnetised magnetised left a comment

Choose a reason for hiding this comment

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

fantastic stuff

Copy link
Member

@alco alco left a comment

Choose a reason for hiding this comment

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

🏆

@icehaunter icehaunter force-pushed the ilia/vax-1289-unsubscribe-handling branch from ec9c694 to bb7f5db Compare May 16, 2024 08:27
@icehaunter icehaunter merged commit a8eedad into main May 16, 2024
12 of 13 checks passed
@icehaunter icehaunter deleted the ilia/vax-1289-unsubscribe-handling branch May 16, 2024 08:49
alco pushed a commit that referenced this pull request May 19, 2024
1. Adds new boundary messages to the protocol allowing the server to
send a GONE batch - a set of messages for the client to know that those
rows will not receive further updates after an unsubscribe call.
2. Server is the source of truth on which rows the client should have
3. Only while-online unsubscribes are possible
4. Client may reconnect at any point after sending the unsubscribe call
and the server will adjust seen rows correctly and send a GONE batch if
we don't believe the client had seen that -- see tests in
`subscriptions_test.exs`.

Missing is a public API for unsubscribes, that's in the next commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants