turbofish-org / merk

High-performance Merkle key/value store
Apache License 2.0
226 stars 36 forks source link

Fetch Trait #13

Closed davebryson closed 5 years ago

davebryson commented 5 years ago

Just trying to follow along with your direction... It looks like you may be removing the get_node closure in favor of a Trait. Is that correct?

I've been experimenting with something similar on my own local branch. Removing get_node for a TreeReader trait. Something like this (although I broke things pretty bad in the process):

/// To be implemented by the backend storage and passed to the tree: &dyn TreeReader
pub trait TreeReader: Send + Sync {
    /// An encoded node from storage
    fn get_node(&self, key: &[u8]) -> Result<Node>;

    /// The root node from storage
    fn get_root_node(&self) -> Option<Node> {
        match self.get_node(&ROOT_KEY_KEY) {
            Ok(node) => Some(node),
            _ => None
        }
    }

    // The value for a given key
    fn get(&self, key: &[u8]) -> Option<Vec<u8>> {
        match self.get_node(key) {
            Ok(node) => Some(node.value),
            _ => None,
        }
    }
}

There'd be a corresponding TreeWriter. Both could then be implemented by RocksDB, etc... Is this kind of trait approach direction you're heading?

mappum commented 5 years ago

It looks like you may be removing the get_node closure in favor of a Trait. Is that correct?

That's right, it feels a little more rust idiomatic to pass around a struct which has a trait method, rather than passing around a closure.

There'd be a corresponding TreeWriter. Both could then be implemented by RocksDB, etc... Is this kind of trait approach direction you're heading?

I'm not a fan of trait-ifying every DB operation since there are many which will be hard to replicate outside of RocksDB, e.g. checkpointing, optimisitic transactions, etc. I do think it makes sense for reading to be a generic interface however, since we could also maybe pass in a caching layer or something like that.

mappum commented 5 years ago

Closing this, but leaving #11 open for discussion around this kind of database abstraction.