-
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?
Conversation
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check for the treeDeleter
to be a AsynchronousTreeDeleter
? Seems like the logic below works regardless.
|
||
// 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 comment
The 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
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Shoud this fail if --experimental_sandbox_async_tree_delete_idle_threads
is 0?
"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.") |
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.
"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.") | |
"Enables async deletion of sanbox base folders." | |
+ "Requires --experimental_sandbox_async_tree_delete_idle_threads > 0") |
d9ca724
to
084d0ae
Compare
--experimental_sandbox_async_tree_delete_on_first_build
- which will enable async tree deletion on first build.