Send empties during sync based on previous timestamp #220
Conversation
somtochiama
changed the title
There was a problem hiding this comment.
I've added inline comments, but in short: only the source node should dictate the cleared timestamps.
The TRIGGER should be changed back to only clear the current site ID's versions and ignore the rest.
So when a node sends a Sync request to another with the "last cleared ts", it won't send its own timestamp, it will send the last one it has seen coming from this node.
crates/corro-types/src/change.rs
Outdated
| @@ -345,20 +354,21 @@ pub fn store_empty_changeset( | |||
|
|
|||
| // println!("inserting: {new_ranges:?}"); | |||
|
|
|||
| let ts = Timestamp::from(agent.clock().new_timestamp()); | |||
There was a problem hiding this comment.
I don't think we ever want to generate our own timestamp for empties unless we're the node that initially created the change. How else would nodes know which empties they are missing? The last cleared timestamp could be more recent than the last one that was sent from another node.
There was a problem hiding this comment.
Added a few comments, but the big possible problem I see here is: losing the information that we are missing some cleared versions.
If we keep updating the "last cleared ts" on every change we apply, then we don't know if we might've missed a cleared versions message in between.
I think this whole mechanism only works if we only store that information when we synchronize with another node and we store their cleared timestamp specifically. that way, we know for sure we've seen all versions the last time we synced with them.
Furthermore, the timestamps stored in bookkeeping help a bit because it gives us a way to reduce the synchronization bandwidth and processing load in the future. But these timestamps need to keep the original cleared timestamp, they can't be created from the current actor's clock.
In summary:
- if we keep updating the sync table with timestamps as we get them from any node, then we'll miss updates
- we should only update the last cleared ts when we sync and only for the node we're syncing with
- we should keep the original timestamp of the cleared version
crates/corro-types/src/agent.rs
Outdated
| CREATE TEMP TABLE _variables (name TEXT PRIMARY KEY, var TEXT); | ||
| INSERT INTO _variables VALUES ('current_ts', strftime('%Y-%m-%dT%H:%M:%S.000000000Z', CURRENT_TIMESTAMP)); | ||
|
|
||
| UPDATE __corro_bookkeeping SET ts = (SELECT var FROM _variables WHERE name = 'current_ts') WHERE ts IS NULL; |
There was a problem hiding this comment.
Interesting! Is that faster than just using strftime directly instead of a temporary table?
There was a problem hiding this comment.
also: should this only update the current node? the current node is the source of truth for its own clears.
There was a problem hiding this comment.
Interesting! Is that faster than just using strftime directly instead of a temporary table?
I don't think so. I am using a temp table because I want to put the same value in __corro_sync_state for this note. I guess its a big deal if they are slightly diffferent
| .blocking_write("process_multiple_changes(update_cleared_ts)"); | ||
| let mut snap = booked_writer.snapshot(); | ||
| if let Some(ts) = last_cleared { | ||
| snap.update_cleared_ts(&tx, ts) |
There was a problem hiding this comment.
the one above if for the received empties. this one is for the node's own empties (since we check for the node's overwrittten versions and we clear them in this function)
| if process { | ||
| let mut retain_keys = Vec::new(); | ||
| for (actor, changes) in &buf { | ||
| match process_emptyset(agent.clone(), bookie.clone(), *actor, changes.clone()).await |
There was a problem hiding this comment.
since you're awaiting without spawning, you can pass references instead of cloning.
somtochiama
force-pushed
the
sync-cleared-versions
branch
from
4e73de0
to
5e80e0d
Compare
|
superseded by #223 |
This pull requests updates the sync process between nodes to also send previously cleared versions that the client node might have already received. Each time a node stores an empty version, it also stores a timestamp and can use this to know which versions have been cleared or updated since the last sync.