#1: Add FileStorage struct #10
Conversation
|
@Pushkarm029 the changes look good. Please move the logic into a single file in a bin directory. That way cargo knows it's meant to be compiled as an executable. Something like - https://doc.rust-lang.org/cargo/guide/project-layout.html The main argument can choose to read or write based on the pass arguments. You can consider using clap (or similar library) to handle cli arguments. |
|
@twitu it seems that we need to rebase this branch |
|
The |
fs-storage/src/file_storage.rs
Outdated
| pub struct FileStorage { | ||
| log_prefix: String, | ||
| label: String, | ||
| path: PathBuf, | ||
| timestamp: SystemTime, | ||
| } |
There was a problem hiding this comment.
What about such API?
pub struct Mapping<K,V> {
timestamp: SystemTime,
values: HashMap<K, V>
}
pub struct Storage<K,V> {
label: String,
path: PathBuf,
mapping: Option<Mapping<K,V>> // `None` means not loaded yet
}
impl Storage {
...
fn new(label: String, path: &Path) -> Self { ... }
fn load(self: Self) -> Self { ... }
fn update(self: Self) -> Self { ... }
...
}
Or something like this:
pub struct Mapping<K,V> {
timestamp: SystemTime,
values: HashMap<K, V>
}
pub struct Storage<K,V> {
label: String,
path: PathBuf,
mapping: Mapping<K,V>
}
impl Storage {
...
fn provide(label: String, path: &Path) -> Self { ... }
fn update(self: Self) -> Self { ... }
...
}
There was a problem hiding this comment.
I've updated the api to remove the callback style and just return the hashmap directly. Currently I don't see how the additional complexity of wrapping the HashMap in a Mapping will help.
I think the current state of the PR is a good first version of a functional file storage implementation. I propose we merge it and add more features as separate issues. I prefer not to keep PRs open for too long because they lead to merge conflicts.
There was a problem hiding this comment.
Agree about the conflicts. But if the changes are small it's better to make it in same PR to avoid excessive task management.
Mapping wrapper is useful only because it also contains timestamp. We could use a tuple, but named structure is better. The rest looks good for now.
|
Good job so far. Apart from the comments above, here are couple other things to do:
|
fs-storage/src/cli/write.rs
Outdated
| } | ||
|
|
||
| if let Err(err) = write_to_file(kv_pairs, storage_path) { | ||
| println!("Error writing to file: {}", err); |
There was a problem hiding this comment.
Errors should go to stderr using eprintln!()
There was a problem hiding this comment.
Since this a dev cli errors are unwrapped in the new implementation
There was a problem hiding this comment.
Since this a dev cli errors are unwrapped in the new implementation
I second this.
I know it's a grey area in rust where people take sides in general. For our case (CLI), I think we should never panic
I never want to see this output as a user when running a CLI command
thread 'main' panicked at fs-storage/src/bin/cli.rs:54:18:
Failed to read JSON file: Os { code: 2, kind: NotFound, message: "No such file or directory" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtraceI prefer passing down the error or using anyhow::Context
Here is a reference from the "Command Line Applications in Rust" book
https://rust-cli.github.io/book/tutorial/errors.html
What do you think?
|
If the "Verify build" CI test is blocking this, you can cherry pick ba41bd8 to fix it |
|
@twitu The samples work, thanks. I've noticed one more thing to fix:
|
|
I've converted #10 (comment) into separate issues:
But let's have deterministic output in this PR. Also, it seems that this branch and main have same commits but with different hashes. This causes duplicate commits during rebase. Probably we can squash though. |
There was a problem hiding this comment.
I gave the binary a spin locally and everything works as expected. Thanks for adding support for JSON files!
Left some comments
We also need to make sure the CI is green before merging. Currently the benchmarks one is failing but I added a suggestion to fix it
| @@ -0,0 +1,3 @@ | |||
| [toolchain] | |||
| version = "1.75.0" | |||
There was a problem hiding this comment.
Is there a reason for this specific version?
There was a problem hiding this comment.
A good resource: https://gribnau.dev/cargo-msrv/commands/find.html
There was a problem hiding this comment.
The toolchain is what the compiler and tooling for this workspace will default too. Is there any requirement to support older versions? Using older versions can give difficulty later in upgrading libraries. Unless there's some specific requirement to support older versions we should go ahead with the latest and greatest. Especially so because we plan to pull in the tokio and async ecosystem which is still evolving.
|
Let's also rebase the branch on top of main |
Benchmark for 3ef631dClick to view benchmark
|
Benchmark for 5590529Click to view benchmark
|
Benchmark for 49a5a8bClick to view benchmark
|
Benchmark for 4d167a1Click to view benchmark
|
Benchmark for 2379496Click to view benchmark
|
Co-Authored-by: Ishan Bhanuka <[email protected]> Co-Authored-by: Pushkar Mishra <[email protected]> Co-Authored-by: Tarek <[email protected]> Co-Authored-by: Kirill Taran <[email protected]>
Benchmark for 62802d1Click to view benchmark
|
|
Fixed the commit history. I also experimented with creating a mapping struct in separate branch. It adds extra complexity of generics and also a few unresolved problems around consistency of the timestamp. Basically we have two timestamps in the system now. One for the mapping and one for the file storage struct. This leads to duplication without clear distinction between how they are different or what their relation is to each other. Secondly, the save function doesn't actually use the timestamp in the mapping since it reads the current time. The mapping struct is only used in one place where it is returned by the We can consider adding it later when it has more use cases. This is a good first version of the FileStorage logic and we can merge it once @Pushkarm029 gives the cli a final refactor. |
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
Benchmark for ec54217Click to view benchmark
|
Benchmark for a48c673Click to view benchmark
|
Co-authored-by: Tarek Elsayed <[email protected]>
Co-authored-by: Tarek Elsayed <[email protected]>
Co-authored-by: Tarek Elsayed <[email protected]>
Benchmark for 63e38a5Click to view benchmark
|
Benchmark for a78f010Click to view benchmark
|
Benchmark for 20ec58eClick to view benchmark
|
Benchmark for 02c30c8Click to view benchmark
|
Signed-off-by: Tarek <[email protected]>
Benchmark for 4606153Click to view benchmark
|
* Add FileStorage logic, example and documentation Co-Authored-by: Ishan Bhanuka <[email protected]> Co-Authored-by: Pushkar Mishra <[email protected]> Co-Authored-by: Tarek <[email protected]> Co-Authored-by: Kirill Taran <[email protected]> * refactor done Signed-off-by: Pushkar Mishra <[email protected]> * fix cargo.toml Signed-off-by: Pushkar Mishra <[email protected]> * Update fs-storage/src/file_storage.rs Co-authored-by: Tarek Elsayed <[email protected]> * Update fs-storage/src/file_storage.rs Co-authored-by: Tarek Elsayed <[email protected]> * Update fs-storage/src/file_storage.rs Co-authored-by: Tarek Elsayed <[email protected]> * Add doc comment for erase * feat(fs-storage): refactor CLI write cmd to accept key-value pairs Signed-off-by: Tarek <[email protected]> --------- Signed-off-by: Pushkar Mishra <[email protected]> Signed-off-by: Tarek <[email protected]> Co-authored-by: Pushkar Mishra <[email protected]> Co-authored-by: Tarek <[email protected]> Co-authored-by: Kirill Taran <[email protected]> Co-authored-by: Tarek Elsayed <[email protected]>
* Add FileStorage logic, example and documentation Co-Authored-by: Ishan Bhanuka <[email protected]> Co-Authored-by: Pushkar Mishra <[email protected]> Co-Authored-by: Tarek <[email protected]> Co-Authored-by: Kirill Taran <[email protected]> * refactor done Signed-off-by: Pushkar Mishra <[email protected]> * fix cargo.toml Signed-off-by: Pushkar Mishra <[email protected]> * Update fs-storage/src/file_storage.rs Co-authored-by: Tarek Elsayed <[email protected]> * Update fs-storage/src/file_storage.rs Co-authored-by: Tarek Elsayed <[email protected]> * Update fs-storage/src/file_storage.rs Co-authored-by: Tarek Elsayed <[email protected]> * Add doc comment for erase * feat(fs-storage): refactor CLI write cmd to accept key-value pairs Signed-off-by: Tarek <[email protected]> --------- Signed-off-by: Pushkar Mishra <[email protected]> Signed-off-by: Tarek <[email protected]> Co-authored-by: Pushkar Mishra <[email protected]> Co-authored-by: Tarek <[email protected]> Co-authored-by: Kirill Taran <[email protected]> Co-authored-by: Tarek Elsayed <[email protected]>
FileStorage struct retains core functionality from Kotlin implementation. #1