-
-
Notifications
You must be signed in to change notification settings - Fork 54
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
More progress on sync #71
Conversation
@johan-bjareholt I'm still in the middle of this PR, but could you look over the changes I've made so far and give me some feedback on my attempts to unify the datastore and aw-client-rust APIs? I also heard you were working on a migration tool, and the things I'm building here could potentially give that for free. |
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.
Here are my comments about the new trait, didn't look at the rest very thoroughly
} | ||
fn insert_events(&self, bucket_id: &str, events: Vec<Event>) -> Result<Vec<Event>, String> { | ||
let res = self.insert_events(bucket_id, &events[..]).unwrap(); | ||
self.force_commit().unwrap(); |
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.
Don't force commit here
} | ||
fn heartbeat(&self, bucket_id: &str, event: Event, duration: f64) -> Result<Event, String> { | ||
let res = self.heartbeat(bucket_id, event, duration).unwrap(); | ||
self.force_commit().unwrap(); |
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.
Don't force commit here, if you want to force commit only for the syncing code you should do yet another wrapper above Datastore IMO.
use aw_server::models::{Event, Bucket}; | ||
|
||
// This trait should be implemented by both AwClient and Datastore, unifying them under a single API | ||
pub trait AccessMethod: std::fmt::Debug { |
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.
AccessMethod is a very generic name and doesn't describe what it does
@johan-bjareholt The latest commits here fixes aw-server-rust to work with the latest nightly and improves the build process on Android. Can we merge it since the WIP stuff it isolated in a member package anyway? |
@ErikBjare Seems like there is a merge conflict
Yes, thats fine IMO. Better than having a dead branch since it doesn't impact anything else anyway. |
I wanted the aw-sync-rust implementation able to sync between both aw-server-python and aw-server-rust instances so I started working on the ability to use the REST API to get the job done.