Skip to content
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

Recovery #397

Draft
wants to merge 75 commits into
base: master
Choose a base branch
from
Draft

Recovery #397

wants to merge 75 commits into from

Conversation

cassyunknown
Copy link

@cassyunknown cassyunknown commented Feb 11, 2022

Adds support for recovering from file-based datapool, intended for use with PMEM.

@CLAassistant
Copy link

CLAassistant commented Feb 11, 2022

CLA assistant check
All committers have signed the CLA.

.gitignore Outdated Show resolved Hide resolved
src/rust/config/src/seg.rs Show resolved Hide resolved
src/rust/config/src/seg.rs Show resolved Hide resolved
@@ -151,6 +205,24 @@ impl Seg {
pub fn datapool_path(&self) -> Option<PathBuf> {
self.datapool_path.as_ref().map(|v| Path::new(v).to_owned())
}

pub fn segments_fields_path(&self) -> Option<PathBuf> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should each of these paths be it's own config option? I suspect we can be opinionated about the names for each file if we decide to keep the parts separate.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was going to wait until we decided how many files to use

src/rust/entrystore/src/seg/mod.rs Outdated Show resolved Hide resolved
src/rust/storage/seg/src/segments/segments.rs Outdated Show resolved Hide resolved
src/rust/storage/seg/src/segments/segments.rs Outdated Show resolved Hide resolved
src/rust/storage/seg/src/segments/segments.rs Outdated Show resolved Hide resolved
src/rust/storage/seg/src/ttl_buckets/ttl_bucket.rs Outdated Show resolved Hide resolved
src/rust/storage/seg/src/ttl_buckets/ttl_buckets.rs Outdated Show resolved Hide resolved
.github/workflows/cmake.yml Outdated Show resolved Hide resolved
/// Copies `size` bytes at `byte_ptr` to the `offset` of `data`
/// Returns the next `offset`, that is, the next byte of `data` to be copied into
pub fn store_bytes_and_update_offset(byte_ptr: *const u8, offset: usize, size: usize, data: &mut [u8]) -> usize {
// get corresponding bytes from byte pointer
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put this function here so that Segments, TtlBuckets and HashTable could all access it. I am slightly concerned about security as I am not sure if this could be used as a "write what I want where I want"


if config.graceful_shutdown() {
// TODO: check if successfully shutdown and record result
self.data.flush();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line currently produces a warning as I am ignoring the Result. Ideally the result would be intepreted here or passed along to server/segcache/src/lib.rs where I intend this flush() function to be called


/// Flush (gracefully shutdown) the `Seg` cache if configured to do so
pub fn flush<T: SegConfig>(self, config: &T) {
let config = config.seg();
Copy link
Author

@cassyunknown cassyunknown Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find a trait that I should implement (that only has the flush() required method) so I just put this function as a method of Seg. I did the same thing for the other files I added a flush() to

… not point in keeping this until there is a nice way to determine expected file size
…ll change code so that Segments use same file. Also need to tidy code up
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants