-
Notifications
You must be signed in to change notification settings - Fork 247
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
Segment headers store #1692
Segment headers store #1692
Conversation
…ng storage and in-memory cache of segment headers (not used yet)
…tHeaderProvider`
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.
I got a couple of questions about ever-growing memory and cache loading times. Also, I would like to see debug messages in methods that have a chance to fail (like disk IO).
let insert_data = vec![(key.as_slice(), value.as_slice())]; | ||
|
||
self.inner.aux_store.insert_aux(&insert_data, &[])?; | ||
self.inner.cache.lock().extend_from_slice(segment_headers); |
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.
Does it mean ever-growing memory usage? How do you estimate memory usage during the network life?
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.
Yes, it is estimated to be less than 1 GiB for 1 PiB of pieces, so should not be an issue for a very long time. We can always switch to loading data from disk, but it is much better to have in-memory cache.
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.
Also, I would like to see debug messages in methods that have a chance to fail (like disk IO).
There are already errors returned already and they are logged at call site, is there something else you have in mind?
let insert_data = vec![(key.as_slice(), value.as_slice())]; | ||
|
||
self.inner.aux_store.insert_aux(&insert_data, &[])?; | ||
self.inner.cache.lock().extend_from_slice(segment_headers); |
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.
Yes, it is estimated to be less than 1 GiB for 1 PiB of pieces, so should not be an issue for a very long time. We can always switch to loading data from disk, but it is much better to have in-memory cache.
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.
There are already errors returned already and they are logged at call site, is there something else you have in mind?
That will be enough for this PR. Consider adding more logging for complex code using trace
or debug
log levels in the future. It could be easier to understand what is going on in the app when debugging.
This implements global persistent segment headers store (with in-memory cache) in node and replaces a few different stores we used before this.
Implementation will be extended in the future, in particular it will be possible to store sequences of segment headers in the same entry (not implemented, but there is TODO for this that I called compaction) to reduce fragmentation as well as populate segment headers store initially when syncing from DSN (in which case we'll have to check that segment headers are written sequentially, there is TODO about this as well).
Data stored on disk changes in this PR, hence breaking label.
Resolves #1125
Code contributor checklist: