-
Notifications
You must be signed in to change notification settings - Fork 119
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
Conversation
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.
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.
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.
excellent work. you do need to fix those protobuf id conflicts tho.
components/electric/lib/electric/satellite/client_reconnection_info.ex
Outdated
Show resolved
Hide resolved
components/electric/lib/electric/satellite/client_reconnection_info.ex
Outdated
Show resolved
Hide resolved
components/electric/lib/electric/satellite/client_reconnection_info.ex
Outdated
Show resolved
Hide resolved
components/electric/lib/electric/satellite/client_reconnection_info.ex
Outdated
Show resolved
Hide resolved
CREATE TABLE #{Extension.client_unsub_points_table()} ( | ||
client_id VARCHAR(64) NOT NULL, | ||
subscription_id UUID NOT NULL, | ||
wal_pos BIGINT NOT NULL, |
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 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. |
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'm not sure what this says. "Get off that fake root"? At what point later?
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.
fantastic stuff
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.
🏆
…e subscription ID
… we receive additional data. We filter the graph before saving it to additional data.
ec9c694
to
bb7f5db
Compare
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.
subscriptions_test.exs
.Missing is a public API for unsubscribes, that's in the next PR.