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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fs-storage
: Add FolderStorage struct #81fs-storage
: Add FolderStorage struct #81Changes from 4 commits
bef8c7d
181428a
afa0b37
7600ab4
5ebb50c
7af0094
3c82844
46f53f9
004de87
353379e
0bdd7ad
68ea331
c53a0b4
283f842
887a0d2
aba82ac
f607c3e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the key in
FolderStorage
is the filename, we should add a constraint thatK
must implementstd::fmt::Display
, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Technically, we could solve the task for arbitrary key type by storing the keys together with values. But this complicates the solution unnecessary for this moment. We'll be able to loose the constraint when we implement "chunked storage".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ChunkedStorage is meant to store n key-value pairs per file. FolderStorage is a special case where n = 1. We can optimise the special case by storing the key only in the in-memory mapping. This means that the key only needs constraints to be stored in the BTreeMap which would be Ord + Eq.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@twitu Yes, but the easiest way to persist the keys is in the filenames.
"qwe"
,"asd"
,"zxc"
are the keys here, e.g. file<storage_root>/qwe
contains value assigned to key"qwe"
Do we store data like this currently? We could just fix the keys type to
String
but we want to use the storage at least with numerical keys, too. So,Display
is the most practical generalization.If we allow keys to be just
Ord
, then we need to serialize, store, load and deserialize them. I would postpone such work till we start working on the chunked storage.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where should we store the version information? In
FileStorage
, it is serialized in a separate field. ForFolderStorage
, we need a way to track the version to ensure backward compatibility in the future. I have two ideas:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Files storing the values should not be used for this. Examples of values stored in a
FolderStorage
range from JSON properties to binary image data.Storing the version in a separate file sounds like a better idea to me. But do we really need versioning here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might manage without versioning information in
FolderStorage
, but if we decide to update to a newer version later (for instance, if binary serialization turns out to be unsuitable), implementing backward compatibility will become more challenging, though not impossible.We've already dealt with a similar situation in
FileStorage
.I don't have a strong opinion on this. If you prefer to omit versioning information, that's fine with me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. We might change the folder structure in future, too. Let's add version file just for the case.
On other hand, versioning of file format probably should be delegated to user side, because we should just write bytes using writer that user passes to us. So, JSONs could be printed as text, or in binary form. Can we implement it in a way that user controls how they want to store JSONs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proper way to allow the user to pick the serialization format would be to introduce another generic parameter and allow it to pick from a given format, or by making the generic parameter an implementation of Writer. However, I believe this can be a feature for later.
Currently, we can pick JSON as the default format since it is human readable. In fact, later when we extend the implementation for multiple writers for generics we don't need a separate version file because generic parameters can be set with default as JSON.
So for v1, I think not making it generic and using serde_json as the serialization format will be a good approach.