-
Notifications
You must be signed in to change notification settings - Fork 29
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
async/await: complete rewrite of foundationdb #143
Conversation
- no more indirection within FdbFuture - no race condition possible (Transaction can't be cloned anymore) and commit, cancel, reset are protected - fdb api 610+ - the tuple layer is implemented with serde - option generation is now indented and the code is simpler - RangeOption and KeySelector can be either be Owned or Borrowed - fix KeySelector offset can be negative - some int options can be negative - fix boot api safety (undefined behavior was possible) - simple boot process
Maybe fdb_iteration should panic or handle the error itself. `iteration` is checked in foundationdb and returns client_invalid_operation if it is <= 0 There is no information in the API spec about it. For example if we pass a key.len() < 0 as we could before this commit, it is undefined behavior, because foundationdb is not checking it.
This is simplify the usage of get_ranges by directly giving a stream of keyvalue. This require keeping the whole FDBFuture alive until every FdbFutureValue is dropped.
@brndnmtthws, thoughts? I currently don’t have skin in the game, but this is a significant rewrite and change in direction. |
Well serde was a bad idea to implement the tuple layer, I'm converting it back. |
This latest version passes the following bindingtester tests 100 times (might still miss some low occurrences bugs). ./bindingtester.py --num-ops 1000 --test-name api --api-version 610
./bindingtester.py --num-ops 1000 --concurrency 5 --test-name api --api-version 610
./bindingtester.py --test-name scripted Most bugs were in the tuple pack/unpack code and in the bindingtester code itself. |
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.
Ok, I went through at the given state. Given it's such a massive rewrite, I'm sure I missed something, but otherwise it all looks reasonable.
I'm not sure about the serde depedency and the value add there, but it does look to have simplified the serialization quite a bit.
@yjh0502 do you have any issues with this? @brndnmtthws? @dae, this would supersede your async/await support PR if we decide to go this direction.
@Speedy37 assuming everyone else is ok with this rewrite, I'd say let's merge this. Rather than forking, I'd prefer to make you collaborator or transfer ownership (whatever we think is best), and then I'll add you on crates.io to be able to push new versions.
I haven't reviewed the code properly, but have no objections to my PR being discarded if this is a better long term solution. @Speedy37 : Is the retry/timeout code in transact() necessary? I was under the impression FDB handles that itself when on_error is called. |
I dropped the serde approach to ser/deserialization as some types (Versionstamp, Uuid) were quite hard to support. It might be possible to add it back as I had like to not fork too. I'm also open to suggestion and would recommend not merging this until rustc 1.39 is out. I still need to finish porting my app whole stack to this version to validate the usability of the API. In the current state, I'm not sure about the signature of Database::transact. Fdb does handle the retry count itself if specified, so this limit is a duplicate. |
thread 'main' panicked at 'attempt to subtract with overflow', foundationdb/src/transaction.rs:245:13
Ok this is only missing the directory layer API now and I don't know if I can spend the time to write it soon. Probably next month. |
Here is a link to the latest generated documentation: https://clikengo.github.io/foundationdb-rs/foundationdb/index.html |
Ok for now, the biggest downside of this version is that
|
Changes to how options are named are breakingchanges.
This is awesome - thankyou so much for getting this working! 😍 |
FWIW I think sharing a transaction reference is the right API - getting static lifetime checks is good rust code, even if it means users need lifetime qualifiers for the transaction object. |
🎉 this is such a huge step forward, amazing work |
I don't know if this commit can be the future of this project or if I should publish it on my own with another name. I changed the API and logic quite a lot.
The most important change to me is how transactions are shared across threads. They can't be cloned anymore, so you share them by reference, Transaction implements Send/Sync, but commit/cancel/reset api requires mutable access to a Transaction. This protect against undefined behavior that was previously possible (cancel/reset) data races.
I'm marking this as WIP as I still need to polish/document it.
Here is a small/incomplete TODO of what I want to do:
I've not focused much on the tuple/subspace/directory API as I'm not using them in production. I built a strictly typed API that enforce database layout.
Fixes #111
Fixes #117
Fixes #123
Fixes #124
Fixes #125
Fixes #128
Fixes #130
Fixes #132
Fixes #135