-
Notifications
You must be signed in to change notification settings - Fork 5
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
[WIP] Async tree deletion on first build #15
base: databricks-bazel-6.3.1
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -238,8 +238,31 @@ private void setup(CommandEnvironment cmdEnv, SpawnStrategyRegistry.Builder buil | |
// previous builds. However, on the very first build of an instance of the server, we must | ||
// wipe old contents to avoid reusing stale directories. | ||
if (firstBuild && sandboxBase.exists()) { | ||
cmdEnv.getReporter().handle(Event.info("Deleting stale sandbox base " + sandboxBase)); | ||
sandboxBase.deleteTree(); | ||
if (options.asyncFirstBuildDelete && (treeDeleter instanceof AsynchronousTreeDeleter)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shoud this fail if |
||
cmdEnv.getReporter().handle(Event.info("Async deleting stale sandbox base " + sandboxBase)); | ||
// Create a staging ground for the old sandbox base to be moved to. | ||
Path oldSandboxBaseStagingGround = sandboxBase.getParentDirectory().getRelative("old-sandbox-staging"); | ||
// Create the staging ground dir if it doesn't exist | ||
oldSandboxBaseStagingGround.createDirectoryAndParents(); | ||
|
||
// Get a random int to use as a unique identifier for this deletion. This should be a | ||
// positive int. nextInt(foo) returns a value between 0 (inclusive) and foo (exclusive). | ||
int identifier = new Random().nextInt(Integer.MAX_VALUE); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's use the current server pid instead, it is guarantee to be unique |
||
Path oldSandboxBase = oldSandboxBaseStagingGround.getRelative("old-sandbox-" + identifier); | ||
|
||
// Move the old sandbox base to the location in the staging ground. | ||
sandboxBase.renameTo(oldSandboxBase); | ||
|
||
// At this point, it's safe for the primary thread to continue - the sandbox base is empty | ||
|
||
// Use the async tree deleter to cleanup all the contents of the staging ground. If there | ||
// were previously staged deletes that didn't complete, they're also in this directory and | ||
// will be cleaned up. | ||
treeDeleter.deleteTreesBelow(oldSandboxBaseStagingGround); | ||
} else { | ||
cmdEnv.getReporter().handle(Event.info("Deleting stale sandbox base " + sandboxBase)); | ||
sandboxBase.deleteTree(); | ||
} | ||
} | ||
firstBuild = false; | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -348,6 +348,17 @@ public ImmutableSet<Path> getInaccessiblePaths(FileSystem fs) { | |||||||||||
+ " grows to the size specified by this flag when the server is idle.") | ||||||||||||
public int asyncTreeDeleteIdleThreads; | ||||||||||||
|
||||||||||||
@Option( | ||||||||||||
name = "experimental_sandbox_async_tree_delete_on_first_build", | ||||||||||||
defaultValue = "false", | ||||||||||||
documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, | ||||||||||||
effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS}, | ||||||||||||
help = | ||||||||||||
"If false, sandbox deletion on the first build will be done synchronously. If true, " | ||||||||||||
+ "and experimental_sandbox_async_tree_delete_idle_threads is non-0, sandbox deletion" | ||||||||||||
+ "on the first build will be done asynchronously.") | ||||||||||||
Comment on lines
+357
to
+359
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
public int asyncFirstBuildDelete; | ||||||||||||
|
||||||||||||
@Option( | ||||||||||||
name = "incompatible_legacy_local_fallback", | ||||||||||||
defaultValue = "true", | ||||||||||||
|
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.
Do we need to check for the
treeDeleter
to be aAsynchronousTreeDeleter
? Seems like the logic below works regardless.