-
Notifications
You must be signed in to change notification settings - Fork 7
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
Real-time Sync: Add push logic and run method #568
base: yse-rt-merge
Are you sure you want to change the base?
Conversation
self.persister.get_sync_state_by_record_id(&record_id), | ||
"Could not retrieve sync state: {}" | ||
) else { | ||
log::debug!("Cannot sync record without any sync state. Skipping."); |
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.
When clients upgrade the sdk how would we push their existing swaps (first revision)?
One way is to add a migration step to add sync state for all existing swaps. The other option is to include get_sync_outgoing_details all swaps that without sync details.
I somehow like the second option because:
- It doesn't need any migration step
- We don't need to add sync_state on every time we adda swap and it helps us also separate the sync logic from the business logic.
@@ -208,6 +227,124 @@ impl SyncService { | |||
|
|||
Ok(()) | |||
} | |||
|
|||
async fn push(&self) -> Result<()> { | |||
let outgoing_details = self.persister.get_sync_outgoing_details()?; |
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.
Perhaps we can have one method: get_records_to_push
which will return all records with revisions we need to push?
} in outgoing_details | ||
{ | ||
// Step 1: Get the sync state, ensure it exists | ||
let Some(sync_state) = unwrap_or_continue!( |
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.
Since we are iterating each record here I don't think we need the unwrap_or_continue macro repetitive calls.
We can have one method that is called process_push for example which we call here and log on error once if we get error as result.
I think it sill simplify the code.
} | ||
|
||
// Step 7: Update state revision | ||
unwrap_or_continue!( |
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.
Where do we clean the updated_fields?
updated_fields, | ||
commit_time: utils::now(), | ||
}; | ||
let new_state = SyncState { |
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.
As far as I understand the enqueue method should update the update_fields in the sync_outgoing_details so why do we need to update the sync_state ?
Ok(()) | ||
} | ||
|
||
pub(crate) async fn enqueue_record( |
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.
This method is racy. The updated_fields should be updated in the same transaction that we update the swap.
Closes #504.
TODO: