fs-storage : Add FolderStorage struct
#81
Merged
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
Benchmark for 0f6797eClick to view benchmark
|
Benchmark for ec00992Click to view benchmark
|
twitu
requested changes
Jul 24, 2024
Benchmark for 34f12c2Click to view benchmark
|
twitu
reviewed
Jul 24, 2024
Signed-off-by: pushkarm029 <pushkarmishra029@gmail.com>
|
Tests are failing due to this: error: unexpected `cfg` condition value: `unix`
--> fs-atomic-versions/src/atomic/file.rs:3:7
|
3 | #[cfg(target_os = "unix")]
| ^^^^^^^^^^^^^^^^^^
|
= note: expected values for `target_os` are: `aix`, `android`, `cuda`, `dragonfly`, `emscripten`, `espidf`, `freebsd`, `fuchsia`, `haiku`, `hermit`, `horizon`, `hurd`, `illumos`, `ios`, `l4re`, `linux`, `macos`, `netbsd`, `none`, `nto`, `openbsd`, `psp`, `redox`, `solaris`, `solid_asp3`, `teeos`, `tvos`, `uefi`, `unknown`, `visionos`, `vita`, `vxworks`, `wasi`, `watchos`, and `windows` and 2 more
= note: see <https://doc.rust-lang.org/nightly/rustc/check-cfg/cargo-specifics.html> for more information about checking conditional configuration
= note: `-D unexpected-cfgs` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(unexpected_cfgs)]`
|
Benchmark for 2e2d106Click to view benchmark
|
twitu
approved these changes
Jul 27, 2024
Benchmark for 9544429Click to view benchmark
|
Benchmark for 72ac334Click to view benchmark
|
tareknaser
requested changes
Aug 5, 2024
|
Thanks for the review |
Benchmark for 8a9fc06Click to view benchmark
|
Benchmark for 87c3e0fClick to view benchmark
|
Benchmark for c609ab2Click to view benchmark
|
kirillt
reviewed
Aug 18, 2024
kirillt
reviewed
Aug 18, 2024
kirillt
reviewed
Aug 18, 2024
kirillt
reviewed
Aug 18, 2024
kirillt
requested changes
Aug 18, 2024
* Add property test * Use print statements * experimental changes Signed-off-by: pushkarm029 <pushkarmishra029@gmail.com> * fix Signed-off-by: pushkarm029 <pushkarmishra029@gmail.com> * fix Signed-off-by: pushkarm029 <pushkarmishra029@gmail.com> * Refactor to remove unwraps * Fix folderstorage model with strong deletes --------- Signed-off-by: pushkarm029 <pushkarmishra029@gmail.com> Co-authored-by: pushkarm029 <pushkarmishra029@gmail.com>
|
After experimenting with various synchronization models for concurrent operations, the "StrongDelete" approach is coming out to be the most effective in terms of code complexity and efficiency with passing tests. The synchronization model can be stated as below -
|
Benchmark for ff228d7Click to view benchmark
|
Benchmark for 3916630Click to view benchmark
|
Benchmark for d0e0f38Click to view benchmark
|
Signed-off-by: pushkarm029 <pushkarmishra029@gmail.com>
Benchmark for d0d1e04Click to view benchmark
|
Benchmark for 05dbed4Click to view benchmark
|
twitu
approved these changes
Aug 27, 2024
Benchmark for d4ab4a8Click to view benchmark
|
tareknaser
reviewed
Aug 30, 2024
| .get(key) | ||
| .map(|disk_stamp| ram_stamp > disk_stamp) | ||
| }) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
Why are we defaulting to false here?
There was a problem hiding this comment.
btw, defaulting to true also works
There was a problem hiding this comment.
The existing_value_updated triggers a monoidal combine of values for a particular key.
Right now it works because the property based test uses a Dummy value which has a "dumb" monoidal implementation which does nothing i.e. combine(dummy, dummy) = dummy.
However, when we use values like strings or numbers that have useful monoidal implementations, we want to appropriately set the existing_value_updated flag. We want to combine things only if values have diverged in both memory and storage. So the default should be false, if the value is not updated it is simply set and not combined.
| Ok(()) | ||
| } | ||
| /// Resolve differences between memory and disk data | ||
| fn resolve_divergence(&mut self) -> Result<()> { |
There was a problem hiding this comment.
I'm sorry, but could you explain what this method does?
There was a problem hiding this comment.
It is just an enhanced version of 'merge_from' for resolving divergence. The method considers timestamps to detect keys where we need to use monoids to resolve conflicts.
Added this comment here:
/// Resolves discrepancies between in-memory data and disk data by combining or
/// overwriting values based on which version is more recent, ensuring consistency.
tareknaser
reviewed
Aug 30, 2024
Benchmark for 3075beaClick to view benchmark
|
tareknaser
approved these changes
Sep 1, 2024
kirillt
approved these changes
Sep 2, 2024
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.
FileStorage struct retains core functionality from Kotlin implementation. #1