Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

simplify kvdb-rocksdb restore function#8780

Closed
debris wants to merge 11 commits into
masterfrom
fs-swap
Closed

simplify kvdb-rocksdb restore function#8780
debris wants to merge 11 commits into
masterfrom
fs-swap

Conversation

@debris
Copy link
Copy Markdown
Collaborator

@debris debris commented Jun 4, 2018

@debris debris added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jun 4, 2018
@5chdn 5chdn added this to the 1.12 milestone Jun 4, 2018
Copy link
Copy Markdown
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

fs-swap doesn't look atomic. Maybe better randomize the filename witch each run a bit?

@debris debris changed the title kvdb-rocksdb uses fs-swap crate simplify kvdb-rocksdb restore function Jun 5, 2018
@andresilva andresilva added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 12, 2018
@debris debris added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jun 13, 2018
@debris
Copy link
Copy Markdown
Collaborator Author

debris commented Jun 13, 2018

pr is ready for another review. fs-swap is now atomic, but it may fail if:

a) osx is not using APFS (which is the default)
b) windows directories or on different volumes

in these cases we're trying to do nonatomic swap

Copy link
Copy Markdown
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

The changes in parity look good to me. Regarding fs-swap:

  • Linux: you pass libc::AT_FDCWD to renameat2 shouldn't these be file descriptors to the paths we want to swap?

@debris
Copy link
Copy Markdown
Collaborator Author

debris commented Jun 13, 2018

from http://man7.org/linux/man-pages/man2/rename.2.html

If oldpath is relative and olddirfd is the special value AT_FDCWD,
then oldpath is interpreted relative to the current working directory
of the calling process (like rename()).

If I understand this correctly, by passing AT_FDCWD, the paths are being resolved as in rename(), so exactly as we want

@andresilva
Copy link
Copy Markdown
Contributor

Makes sense, we want to resolve relative paths using the cwd of the process.

if existed {
fs::rename(&backup_db, &self.path)?;
// ignore errors
let _ = fs::remove_dir_all(new_db);

This comment was marked as resolved.

This comment was marked as resolved.

@debris
Copy link
Copy Markdown
Collaborator Author

debris commented Jun 22, 2018

already merged as a part of #8712

@debris debris closed this Jun 22, 2018
@debris debris deleted the fs-swap branch July 5, 2018 10:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants