Skip to content
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

Why so many traits? #27

Open
Kixunil opened this issue Nov 4, 2017 · 10 comments
Open

Why so many traits? #27

Kixunil opened this issue Nov 4, 2017 · 10 comments

Comments

@Kixunil
Copy link
Contributor

Kixunil commented Nov 4, 2017

I'm wondering why there are so many traits in the crate. Traits are used for abstractions, but from what I've seen they don't abstract anything. Wouldn't it be simpler to not have them? Or, if abstraction is desirable, move the traits into separate crate and generalize them little bit more. This way they could be implemented for various databases. (So people who want to use other databases don't need to have leveldb in their dependencies.)

I imagine something like this:

trait KVDatabase {
    type BorrowedKey;
    /// Value that is returned from `get` method
    type OwnedValue: Borrow<Self::BorrowedValue>;
    /// Value that can be passed to `put` and `delete` methods
    type BorrowedValue;
    type GetError;
    type PutError;
    type DeleteError;

    fn get(&self, &Self::BorrowedKey) -> Result<Option<Self::OwnedValue>, Self::GetError>;
    fn put(&mut self, &Self::BorrowedKey, Self::BorrowedValue) -> Result<(), Self::PutError>;
    fn delete(&mut self, &Self::BorrowedKey) -> Result<bool, Self::DeleteError>;
}

trait IterKVDatabase: KVDatabase {
    type OwnedKey: Borrow<Self::BorrowedKey>;
    type KVIterator: Iterator<Item=(Self::OwnedKey, Self::OwnedValue)>;
    type KeyIterator: Iterator<Item=Self::OwnedKey>;
    type ValueIterator: Iterator<Item=Self::OwnedValue>;

   fn iter(&self) -> Self::KVIterator;
   fn iter_keys(&self) -> Self::KeyIterator;
   fn iter_values(&self) -> Self::ValueIterator;
}
@Kixunil
Copy link
Contributor Author

Kixunil commented Nov 4, 2017

While refactoring iterator code (see PR #28) I realized you probably wanted to avoid duplication (thinking inheritance-based OOP?). This could be achieved more cleanly using contain and delegate pattern. I'm willing to refactor it if you agree.

@skade
Copy link
Owner

skade commented Nov 6, 2017

Phew, so, if you have a look at the initial commit date (March 22, 2014), this library not only predates Rust 1.0, but was also the first larger library I wrote on. So you can flag a lot of parts as "learning to build interfaces over C libs". It is (mostly) an exercise.

The trait separation is indeed not to fake inheritance, but to separate independent interfaces. It makes sure that things only rely on a subset of the rest of the library. I don't quite agree that traits are only for abstraction, but that's certainly up for debate. I would probably not write this interface again, though.

The full reasoning for this? Well, probably lost in time. :)

I deliberately didn't want this library as a low-level leveldb interface with external crates providing a higher one, this is the road https://github.com/andrew-d/leveldb-rs takes.

As a quick fix for user annoyance, probably a prelude module for simple import of everything would make sense.

I agree that this could be more general (a hint of that can be found in the fact that the key type is in its own library). But mostly, I stopped developing this library and just maintain whatever patches come into it. This is mostly due to leveldb itself being superseded by rocksdb anyways and me rarely using it :).

I also wouldn't like to fundamentally break the interface before doing some kind of major jump.

That being said, if you'd like to take the library and work on improvements, I'll happily do reviews and answer questions. I can also give you commit access if you want to.

@skade
Copy link
Owner

skade commented Nov 6, 2017

Out of curiosity, I just looked it up: the design of this interface even predates associated types, which landed around January 2015. So, considering it "somewhat antique" is probably a good way of approaching it ;).

https://github.com/rust-lang/rfcs/blob/master/text/0195-associated-items.md

@Kixunil
Copy link
Contributor Author

Kixunil commented Nov 9, 2017

Hmm, I'm not sure if advance and similar low-level methods are necessary on iterators. Admittedly, one can skip things faster, but another problem I see is allocation. I'd like to see an internal iterator giving slices of data. I'll need it because I have a large dataset in leveldb already (Bitcoin's UTXO set).

Fortunately, this project of mine isn't too urgent.

@skade
Copy link
Owner

skade commented Nov 10, 2017

I'd be very happy with any kinds of API extensions that make the library more useful, especially in data-heavy environments. The only thing I have is that I'd like to not slowly break the interface for existing users, but this could certainly be done by deciding to mark a stable interface at some point and then moving towards a 2.0.

@kaimast
Copy link

kaimast commented Aug 5, 2020

Can I just add on to this that abstraction isn't really necessary for a low-level library like leveldb, especially if development stopped and you do not plan to add support for, let's say, RocksDB.

If you would be willing to accept a pull request, I can help and try to clean up the library a little. If you are too worried about breaking the API, I could also just fork it.

@skade
Copy link
Owner

skade commented Sep 29, 2020

@kaimast leveldb-sys is the library you are looking for then as a base library, which I still maintain. leveldb is mostly in this state because of broad usage. The abstraction is entirely intentional though at a lot of places, as the leveldb C api exposes a lot of concepts in a very low level fashion that Rust has high-level concepts for. Iterators are the prime example and the (documentation-mandated) lifetime relationship between the LevelDB core type and the things it produces (like the iterators), too. My main gripe about the current interface is using traits where not needed, the library should not define traits that no one else implements.

On the rocksdb note: there are rocksdb crates out there and I don't feel like a unified interface over both is useful. Final products will use one over the other for reasons other than the Rust interface and switching between the two is done for effect. The practical benefit of maintaining support for both is small in my reading.

TBH, I don't believe the current leveldb interface is gradually amendable and starting fresh is probably better. Happy to answer questions, also through another channel (I'm @skade on discord and Florian Gilcher on the rust-lang zulip).

@Kixunil
Copy link
Contributor Author

Kixunil commented Sep 30, 2020

The practical benefit of maintaining support for both is small in my reading.

Maybe not. rocksdb takes insanely long time to build and sometimes fails to build or crashes (mainly on RPi). OTOH, it's supposed to be more performant. Therefore using leveldb for development and switching to rocksdb in production might be useful.

Disclaimer: I'm not an expert on their interfaces so it still might not make sense.

@skade
Copy link
Owner

skade commented Sep 30, 2020

@Kixunil RocksDB has a much expanded API over LevelDB. Just as an illustration, the number of exported types is quite a bit larger. https://github.com/facebook/rocksdb/blob/master/include/rocksdb/c.h#L71-L127

Also, functions as basic as get and put have a second form for using specific rocksdb features.

Any library wanting to support both will have to go for the lowers common denominator. Given that you usually choose rocksdb because of the extended functionality, I don't feel this merits a lot of work,

@Kixunil
Copy link
Contributor Author

Kixunil commented Oct 1, 2020

Thanks for explaining! IDK why I got the impression that the difference is just performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants