Conversation
… entrypoint for syncing binary
|
@johan-bjareholt Could I get some comments on this? |
johan-bjareholt
left a comment
johan-bjareholt
left a comment
There was a problem hiding this comment.
I need some more time to look into this, still don't fully understand what's going on.
johan-bjareholt
left a comment
johan-bjareholt
left a comment
There was a problem hiding this comment.
A good start.
We need some restructuring later though, such as move this out to a seperate "sync" folder instead of being inside "src" and then moving out everything in "src" into "core" and "server" or something like that. We don't want everything to become a huge monolith. This doesn't need to be done in this PR.
It would be good if you could modularize it a bit more and write it as multiple tests instead and just keep a non-functional main function for now until everything is properly implemented.
I've looked it though a bit and I think I (kind of) understand the flow of the program, I'm not sure if it's simply because I don't fully understand how it's supposed to work or if the functions are oddly split so I have a hard time understanding the overview.
| } | ||
|
|
||
| // Sync events | ||
| // FIXME: Events are not being saved, does the datastore worker need more time before exit? |
There was a problem hiding this comment.
The events are probably not commited.
You could add a "ForceCommit" command to the datastore if you want.
In addition you could also add an "Stop" command which finishes up and then exits the thread and then the requester thread can wait for the worker thread to exit.
There was a problem hiding this comment.
I fixed it by simply moving the references to the datastores out of the main function and then leaving a sleep in main to give it some time to commit (is this necessary though?).
There was a problem hiding this comment.
While that kinda fixed it, it still leaves all kinds of unexpected behavior so until we add a ForceCommit or something I've just set commit: true when initializing the DatastoreInstance for now.
There was a problem hiding this comment.
Nevermind, added the ForceCommit, was really easy.
There was a problem hiding this comment.
Still leads to loads of unexpected behavior though. For example: If I do a insert_events and then a get_event_count I expect it to return the new number, not the old one (there are a lot of cases like this).
The solution as I see it is to commit pending operations before a get operation. (or on every non-heartbeat command)
I would expect that if we ran the aw-server/aw-client integration tests targeting aw-server-rust we'd trigger many of these weird cases.
There was a problem hiding this comment.
The solution as I see it is to commit pending operations before a get operation.
This is how the sqlite storage method in python does it, might be a good idea yes.
| Ok(Responses::BucketMap(ds.get_buckets())) | ||
| }, | ||
| Commands::InsertEvents(bucketname, events) => { | ||
| ds.commit = true; |
There was a problem hiding this comment.
I won't until we've fixed the commit-before-get. If I do I have to put ds.force_commit() after practically every ds.insert_events() which is really annoying and hard to debug.
| // Needed to give the datastores some time to commit before program is shut down. | ||
| // 100ms isn't actually needed, seemed to work fine with as little as 10ms, but I'd rather give | ||
| // it some wiggleroom. | ||
| std::thread::sleep(std::time::Duration::from_millis(100)); |
There was a problem hiding this comment.
In C you don't need to sleep but can wait for a thread to close down.
How to do it properly in Rust without any risk of deadlocks though I have no idea...
There was a problem hiding this comment.
If you find a way we could always wait and have a timeout to avoid a deadlock.
|
|
||
| /// Returns the sync-destination bucket for a given bucket, creates it if it doesn't exist. | ||
| fn get_or_create_sync_bucket(bucket_from: &Bucket, ds_to: &Datastore) -> Bucket { | ||
| // Ensure the bucket ID ends in "-synced" |
There was a problem hiding this comment.
Would be cool with a "synced" property to the buckets instead.
johan-bjareholt
left a comment
johan-bjareholt
left a comment
There was a problem hiding this comment.
How is this all supposed to work when syncing while aw-server is running?
Will there be writes to the same sqlite file from two processes?
|
@johan-bjareholt The syncing could either happen in the same process, or in a separate process if all write-access to the local |
johan-bjareholt
left a comment
johan-bjareholt
left a comment
There was a problem hiding this comment.
After you have fixed the commit before get we can merge this.
The syncing code still needs some improvement, but as long as we don't break aw-server I'm fine with having merged prototype code.
|
@johan-bjareholt If it's not too much effort I'd like it if you fixed the commit-before-get. It shouldn't be too much work and I think you'd do a much better job (not to mention you'd do it a lot quicker). |
|
@ErikBjare Have you tried to change the transaction behavior to IMMEDIATE instead of EXCLUSIVE? |
Nope, the SQLite docs say that "EXCLUSIVE and IMMEDIATE are the same in WAL mode". Can I merge this so we can get on with merging #65? |
|
Feel free to merge, I don't approve of the syncing code yet but that's just a prototype. I don't approve of the ds.commit=true either but it's not broken and I can fix that in #65 so it's fine. You just need to fix some conflict? |
2 participants
Started working on syncing