RFC: storage
#47
RFC: storage
#47
Conversation
|
|
||
| #[cfg(feature = "rocks_db")] | ||
| pub struct Storage { | ||
| inner: ::rocksdb::DB // The storage backend |
There was a problem hiding this comment.
Perhaps it should be the inner field that has the feature flag?
There was a problem hiding this comment.
Interesting, I agree it makes more sense.
| /// It starts RocksDB instance and then initialize the required column familes | ||
| async fn start(config_path: String) -> Result<Self, Box<dyn Error>> { | ||
| let config_as_string = fs::read_to_string(config_path)?; | ||
| let config: config::Config = toml::from_str(&config_as_string)?; |
There was a problem hiding this comment.
Where is this Config type from? The config crate? If so, it might make more sense to have the config loading abstracted such that storage API users can customise it.
There was a problem hiding this comment.
Yes it's already abstracted using feature, you can check that in storage branch.
| )?; | ||
| Ok(()) | ||
| } | ||
| async fn find_by_hash(hash: Hash, storage: &Storage) -> Result<Option<Self>, OpError> { |
There was a problem hiding this comment.
Hash should be passed by reference to avoid unnecessary cloning (I think Hash implements Deref<Trits> now, so you can just call .encode() on it directly.
There was a problem hiding this comment.
Perfect, yeah I will force reference everywhere.
Rendered