fs-storage: Port BaseStorage abstraction from Kotlin
#23
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
Benchmark for 6ecc575Click to view benchmark
|
Benchmark for 3fa6db1Click to view benchmark
|
Signed-off-by: Pushkar Mishra <[email protected]>
Benchmark for 2966539Click to view benchmark
|
|
Good start, let @tareknaser know please, when the todos are solved and ready for review. |
Signed-off-by: Pushkar Mishra <[email protected]>
Pushkarm029
requested review from
tareknaser
and removed request for
tareknaser
kirillt
reviewed
Apr 10, 2024
kirillt
reviewed
Apr 10, 2024
twitu
reviewed
Apr 10, 2024
Signed-off-by: Pushkar Mishra <[email protected]>
Benchmark for b50e471Click to view benchmark
|
tareknaser
reviewed
Apr 12, 2024
|
I noticed that the current output from It's also worth noting that the file doesn't have {
"version": 2,
"data": {
"a": "1",
"b": "2",
"c": "3"
}
}This is not relevant to changes in this PR but we should fix it later |
|
Good note about JSON, the version header certainly came from our legacy format. We should convert it into JSON field. And let's bump the version number to |
Signed-off-by: Pushkar Mishra <[email protected]>
Benchmark for 9e73735Click to view benchmark
|
twitu
reviewed
Apr 15, 2024
Signed-off-by: Pushkar Mishra <[email protected]>
Benchmark for c1eb286Click to view benchmark
|
twitu
approved these changes
Apr 16, 2024
Signed-off-by: Pushkar Mishra <[email protected]>
Benchmark for cf85a86Click to view benchmark
|
Signed-off-by: Pushkar Mishra <[email protected]>
Benchmark for fd6373aClick to view benchmark
|
twitu
approved these changes
Apr 17, 2024
tareknaser
reviewed
Apr 18, 2024
Signed-off-by: Pushkar Mishra <[email protected]>
Benchmark for b61245fClick to view benchmark
|
tareknaser
reviewed
Apr 19, 2024
fs-storage/src/file_storage.rs
Outdated
| file_storage.data = match file_storage.read_fs() { | ||
| Ok(data) => data, | ||
| Err(err) => { | ||
| log::error!("Error reading storage file: {}", err); |
There was a problem hiding this comment.
If it's the first time creating a FileStorage instance in the specified path, we currently log this as an error.
This is actually a valid scenario and shouldn't be treated as an error.
There was a problem hiding this comment.
Indeed, the first time a FileStorage is created the file doesn't exist. Perhaps that can be checked here. If the file doesn't exist when creating the FileStorage it's being created for the first time. So it can start with an empty BTreeMap.
| @@ -219,4 +226,28 @@ mod tests { | |||
| } | |||
| assert_eq!(storage_path.exists(), false); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_file_storage_is_storage_updated() { | |||
There was a problem hiding this comment.
This test fails on my machine at least.
Output:
failures:
---- file_storage::tests::test_file_storage_is_storage_updated stdout ----
thread 'file_storage::tests::test_file_storage_is_storage_updated' panicked at fs-storage/src/file_storage.rs:240:9:
assertion `left == right` failed
left: true
right: false
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
file_storage::tests::test_file_storage_is_storage_updated
There was a problem hiding this comment.
Tracing the error back. seems like the it is because the nanoseconds don't match. SystemTime stores the time in nanosecond precision
running 1 test
Self timestamp: SystemTime { tv_sec: 1713553086, tv_nsec: 320082935 }
File timestamp: SystemTime { tv_sec: 1713553086, tv_nsec: 320194727 }
thread 'file_storage::tests::test_file_storage_is_storage_updated' panicked at fs-storage/src/file_storage.rs:243:9:
assertion `left == right` failed
left: true
right: falseI don't think we need nanosecond precision. Maybe we can just compare seconds
fn is_storage_updated(&self) -> Result<bool> {
let file_timestamp = fs::metadata(&self.path)?.modified()?;
let file_time_secs = file_timestamp
.duration_since(SystemTime::UNIX_EPOCH)
.unwrap() // SAFETY: We can safely unwrap because the timestamp is always after the epoch
.as_secs();
let self_time_secs = self
.timestamp
.duration_since(SystemTime::UNIX_EPOCH)
.unwrap()
.as_secs();
Ok(file_time_secs > self_time_secs)
}But you might have a better idea.
Signed-off-by: Pushkar Mishra <[email protected]>
Benchmark for f4f7bd8Click to view benchmark
|
tareknaser
approved these changes
Apr 21, 2024
kirillt
pushed a commit
that referenced
this pull request
Apr 25, 2024
--------- Signed-off-by: Pushkar Mishra <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #18