[WIP] Async tree deletion on first build #15
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -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.
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.
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.
Shoud this fail if --experimental_sandbox_async_tree_delete_idle_threads is 0?
Comment on lines
+357
to
+359
| "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.
Suggested change
| "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") |
apattidb
force-pushed
the
databricks-bazel-6.3.1
branch
3 times, most recently
from
d9ca724
to
084d0ae
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
--experimental_sandbox_async_tree_delete_on_first_build- which will enable async tree deletion on first build.