chore(electric): Load and clear client reconnection info per client #1188
Conversation
alco
force-pushed
the
alco/vax-1804-scope-client-reconnection-info
branch
2 times, most recently
from
0ed48fe
to
4169713
Compare
alco
changed the title
alco
force-pushed
the
alco/vax-1804-scope-client-reconnection-info
branch
from
e3973a7
to
e83d160
Compare
components/electric/lib/electric/satellite/client_reconnection_info.ex
Outdated
Show resolved
Hide resolved
alco
force-pushed
the
alco/vax-1804-scope-client-reconnection-info
branch
2 times, most recently
from
e87a7c2
to
2c31b6d
Compare
|
|
||
| # Make sure we don't have any leftover cached info for this client in case its local data | ||
| # was cleared without the server knowing about it. | ||
| ClientReconnectionInfo.clear_all_data!(state.origin, state.client_id) |
There was a problem hiding this comment.
Why do we need to add this here, given :perform_initial_sync_and_subscribe handler will call ClientReconnectionInfo.store_initial_checkpoint!/4 that will clear all this data atomically anyway?
There was a problem hiding this comment.
It's a fair point. I added this call because it looked like a logical thing to do for this module if you don't take into account that ClientReconnectionInfo.store_initial_checkpoint!() calls it internally.
I know that one is required to know what happens in all three modules--WebsocketServer, Protocol, ClientReconnectionInfo--in order to be able to make sense of the data flow for any given client, but I would still argue that having this call here does more good than harm since it's handling an edge case. At some point some implementation detail may change elsewhere that could affect the part of logic defined here, in Protocol. Better be prepared for that than sorry.
| with {:ok, state} <- restore_subscriptions(subscription_ids, state) do | ||
| restore_graph(lsn, observed_txn_data, state) | ||
| end | ||
| end | ||
|
|
||
| defp restore_subscriptions(subscription_ids, %State{} = state) do |
There was a problem hiding this comment.
nitpicky question: what's with function reordering here? Makes it harder to compare diffs
There was a problem hiding this comment.
I simply moved this function definition up because it is called first in restore_client_state(), and restore_graph() is called after it.
This gets rid of the batching and debouncing logic in favour of a simpler approach.
This reverts commit e9ab0c4.
alco
force-pushed
the
alco/vax-1804-scope-client-reconnection-info
branch
from
f269173
to
77c5cd3
Compare
This is a follow-up to #1116, specifically addressing Ilia's request.
Client reconnection info is now reloaded from the database at client connection time. This way we can ensure consistency between the data cached in ETS and data stored in the database.