add transaction support #250
Conversation
|
@hjiayz Thanks for putting this together. I took a quick pass over it, but it's a big PR, so I'm going to set aside some time to do a proper review. |
|
@hjiayz I haven't forgotten about doing a review. Been closing out a few other PRs I was directly involved in. Also, CI builds now have |
resolve conflicts
add test doc fix bug fix snapshot mode
There was a problem hiding this comment.
Great work @hjiayz! I finally got some time to do a review. One other thing I would suggest: either consider how implementing TransactionDB would impact your design, or if you are so inclined, go ahead and implement TransactionDB as part of this PR.
|
@aleksuss As requested, I submitted my review. Thanks for the reminder! |
Co-Authored-By: hjiayz <hjiayz@gmail.com>
…d all tests, including doc test to use it...
| /// Wrapper around RocksDB PinnableSlice struct. | ||
| /// | ||
| /// With a pinnable slice, we can directly leverage in-memory data within | ||
| /// RocksDB toa void unnecessary memory copies. The struct here wraps the |
|
Any update? |
| } | ||
| } | ||
|
|
||
| pub fn put_cf<K, V>(&mut self, cf: ColumnFamily, key: K, value: V) -> Result<(), Error> |
There was a problem hiding this comment.
Interface around ColumnFamily should be consistency.
should be cf: &ColumnFamily here.
| } | ||
| } | ||
|
|
||
| pub fn merge_cf<K, V>(&mut self, cf: ColumnFamily, key: K, value: V) -> Result<(), Error> |
There was a problem hiding this comment.
should be cf: &ColumnFamily
| } | ||
| } | ||
|
|
||
| pub fn delete_cf<K: AsRef<[u8]>>(&mut self, cf: ColumnFamily, key: K) -> Result<(), Error> { |
There was a problem hiding this comment.
should be cf: &ColumnFamily
| /// keys exist in the range ["begin_key", "end_key"). | ||
| pub fn delete_range_cf<K: AsRef<[u8]>>( | ||
| &mut self, | ||
| cf: ColumnFamily, |
There was a problem hiding this comment.
should be cf: &ColumnFamily
|
@hjiayz thanks for putting the effort into this PR. I'm sure I'm not the only one eagerly awaiting this merge. I'm dying to use RocksDB as part of a Rust project but the lack of transactions has been a deal breaker up to this point. Is there any way I can help to move this forward? It looks like it's almost there with just some minor requests outstanding... |
| // needed to disambiguate directories when running in | ||
| // a multi-threaded environment (eg. `cargo test`) since | ||
| // there is no guarantee time will be unique | ||
| let path_num = PATH_NUM.fetch_add(1, Ordering::SeqCst); |
There was a problem hiding this comment.
Do you really think it is a necessary variable for creation unique path? Maybe a time would be enough ?
| } | ||
| } | ||
|
|
||
| impl Open for TransactionDB {} |
There was a problem hiding this comment.
Is there a reason OpenCF is not implemented for TransactionDB?
| } | ||
| } | ||
|
|
||
| pub trait CreateCf { |
There was a problem hiding this comment.
CreateCf, and DropCf, but OpenCF. Consistent capitalization?
|
Any updates on this PR? |
|
hey guys, do you need any help on it? It's kind of important to have |
|
Well, whoever can help with updating and finishing this PR is welcome. The diff is absolutely ridiculous in size, so we'll need reviewers as well. |
|
I think it makes sense to create a new PR for this. This PR seems very huge and includes adding not only transactions support. |
Previous implementation relies on auto compactions to move files to the specific level, which is not reliable. This PR controls the compaction manually to make sure the files will be placed at the expected level.
|
Any updates about this PR? |
|
Closed in favour #565 |
No description provided.