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

Add fs_util gc #6612

Merged
merged 2 commits into from Oct 11, 2018

Conversation

Projects
None yet
2 participants
@illicitonion
Copy link
Contributor

illicitonion commented Oct 9, 2018

Note that this may break all existing open Environments

Add fs_util gc
Note that this may break all existing open Environments

@illicitonion illicitonion requested review from stuhood , ity , blorente and dotordogh Oct 9, 2018

@stuhood

stuhood approved these changes Oct 9, 2018

Copy link
Member

stuhood left a comment

Looks good, although I wonder whether the COMPACT mode is strictly necessary (now).

@@ -182,6 +182,16 @@ to this directory.",
true,
)),
)
.subcommand(
SubCommand::with_name("gc")
.about("Garbage collect the on-disk store. Note that after running this command, any processes with an open store (e.g. a pantsd) may need to re-initialize their store.")

This comment has been minimized.

@stuhood

stuhood Oct 9, 2018

Member

This should only be the case if the leasing mechanism fails, right?

EDIT: Having read further, I guess the compact strategy will also cause this, but fast should not?

@@ -1019,6 +1052,48 @@ mod local {
pub fn all_lmdbs(&self) -> Vec<(Arc<Environment>, Database, Database)> {
self.lmdbs.values().cloned().collect()
}

pub fn compact(&self) -> Result<(), String> {
for b in 0x00..0x10 {

This comment has been minimized.

@stuhood

stuhood Oct 9, 2018

Member

Should this range be constant? Or the bounds at least?

This comment has been minimized.

@illicitonion

illicitonion Oct 10, 2018

Contributor

Pulled out fn

for b in 0x00..0x10 {
let key = b << 4;

let mut dirname = {

This comment has been minimized.

@stuhood

stuhood Oct 9, 2018

Member

Worth adding a tempdir dep? Alternatively, should this fail cleanly if the .tmp directory already exists with something like "compaction already in progress"?

This comment has been minimized.

@illicitonion

illicitonion Oct 10, 2018

Contributor

Done

})?;

let env = Environment::new()
.set_flags(EnvironmentFlags::NO_SYNC | EnvironmentFlags::NO_TLS)

This comment has been minimized.

@stuhood

stuhood Oct 9, 2018

Member

Should extract a constant for the union of these flags.

This comment has been minimized.

@illicitonion

illicitonion Oct 10, 2018

Contributor

Pulled out fn

@illicitonion illicitonion merged commit 7ff58b4 into pantsbuild:master Oct 11, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@illicitonion illicitonion deleted the twitter:dwagnerhall/fs_util/gc branch Oct 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment