search

mit-pdos / noria

Fast web applications through dynamic, partially-stateful dataflow
Apache License 2.0
4.98k stars 242 forks source link

Move back to upstream rust-rocksdb #124

Closed ekmartin closed 5 years ago

ekmartin commented 5 years ago

The only significant change here is that ColumnFamily now has a lifetime parameter which makes it harder to store as a part of PersistentState. I initially threaded through the lifetime parameter up to PersistentState but couldn't get past this error:

error[E0277]: *mut librocksdb_sys::rocksdb_column_family_handle_t cannot be shared between threads safely --> noria-server/dataflow/src/state/persistent_state.rs:70:10

ColumnFamily is marked as Send but I wonder if the way to solve this might be to mark the inner pointer as NonNull in rust-rocksdb? Not sure.

To work around it I changed the column family callsites to instead use db.cf_handle(name), which takes a lock (added in https://github.com/rust-rocksdb/rust-rocksdb/pull/197) and reads from a BTreeMap.

jonhoo commented 5 years ago

This looks good! Out of curiosity, do you think the repeated lock acquisition is going to be a performance bottleneck here at all?

ms705 commented 5 years ago

Changes look good to me -- might be good to get this merged before things diverge too much.

Is there anything that needs doing before?

jonhoo commented 5 years ago

I think the comment I just left is the only real question I have. Some of the code may be possible to simplify a little, but I don't think it's very important.

jonhoo commented 5 years ago

Actually, let's just merge this and then I'll try just moving to a regular dependency.

EDIT: I'm working on the merge now :)

ekmartin commented 5 years ago

Sorry about my unresponsiveness here, and thanks for fixing the git dependency (0.12.2 wasn't released when I opened this PR).