Add --async clean independent of expunge #2053

werkt · 2016-11-07T19:33:39Z

An asynchronous clean without a complete purge and daemon shutdown is
particularly useful when the daemon takes an inordinate amount of time
to read output files for md5sums. Incurs a garbage collector invocation
after cache cleanup in the interest of correct usage reporting.

bazel-io · 2016-11-07T19:33:41Z

Can one of the admins verify this patch?

googlebot · 2016-11-07T19:33:42Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

damienmg · 2016-12-05T10:43:19Z

src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java

@@ -191,6 +227,7 @@ private void actuallyClean(CommandEnvironment env,
    if (cleanOptions.expunge || cleanOptions.expunge_async) {
      throw new ShutdownBlazeServerException(0, ShutdownMethod.EXPUNGE);
    }
+    System.gc();


Why gc here?

We noted specifically that workspace.clearCaches() removed nearly all object utilization, and that a gc invocation was definitive in demonstrating that nearly all of bazel's (vm) persistent heap usage was tied up in the cache. Meaningless in the expunge case, it was just an opportunity to show it had nothing else up its sleeves. I'm fine with removing it, it just felt right.

damienmg · 2016-12-05T10:44:06Z

src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java

-          LOG.finest("Cleaning " + child);
-          FileSystemUtils.deleteTreesBelow(child);
+          if (cleanOptions.async) {
+            LOG.finest("Cleaning " + child + " asynchronously...");


Can we factor this so we do not have code duplication

damienmg · 2016-12-05T10:44:45Z

src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java

        "Starting clean." :
        "Starting clean (this may take a while). " +
-            "Consider using --expunge_async if the clean takes more than several minutes.";
+            "Consider using --async if the clean takes more than several minutes.";


That will be wrong if the person use --expunge right?

damienmg · 2016-12-05T10:45:34Z

src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java

@@ -109,10 +121,10 @@ public ExitCode exec(CommandEnvironment env, OptionsProvider options)
      cleanOptions.cleanStyle = "expunge";
    }

-    String cleanBanner = cleanOptions.expunge_async ?
+    String cleanBanner = (cleanOptions.expunge_async || cleanOptions.async) ?


You also want to duplicate the logic at line 116 to avoid doing async clean on platforms that does not support it.

damienmg · 2016-12-05T10:46:21Z

@werkt Can you sign the CLA or if signed with a corporate CLA signal us?

Sorry for having taken so long to look at this PR.

damienmg · 2016-12-05T10:48:06Z

Jenkins test this please

werkt · 2016-12-06T19:24:12Z

I'm on the corporate CLA - ggensure@uberatc.com (just added as verified email on here), signed and membered of open source group under Uber.

damienmg · 2016-12-06T20:27:32Z

Ok thank you for updating your PR. I did another quick pass and I think the PR is good to go but I will be OOO for the rest of the week so reassigning to @ulfjack so a more throughful review can be done.

Again, I would like to apologize in the name of the whole team for taking so long to look at your change.

ulfjack · 2016-12-06T20:31:08Z

src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java

+
+    // Doesn't throw iff command exited and was successful.
+    new CommandBuilder().addArg(command).useShell(true)
+      .setWorkingDir(tempPath.getParentDirectory())


style: this should be four spaces indentation

ulfjack · 2016-12-06T20:32:14Z

src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java

+        defaultValue = "false",
+        category = "clean",
+        expansion = "--clean_style=async",
+        help = "If specified, clean will asynchronously remove the entire working tree for "


I would use present tense here:
"If specified, %product% asynchronously removes the entire working tree ..."

I realize that the other doc doesn't do that right now, and I think those should be changed as well.

ulfjack

I think the change is reasonable. I'm wondering if we should clean async by default instead, but I'm ok with merging this without having that discussion.

werkt · 2016-12-06T20:44:56Z

Updated for @ulfjack's review. No need to apologize @damienmg, we're all busy people :)

ulfjack · 2016-12-06T21:39:00Z

src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java

@@ -132,6 +150,30 @@ public ExitCode exec(CommandEnvironment env, OptionsProvider options)
    }
  }

+  private void asyncClean(CommandEnvironment env,


Can you make this method static?

ulfjack · 2016-12-06T21:39:42Z

src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java

-          FileSystemUtils.deleteTreesBelow(child);
+          LOG.finest("Cleaning " + child + (cleanOptions.async ? " asynchronously..." : ""));
+          if (cleanOptions.async) {
+            asyncClean(env, child, "Child");


Can you make the string a bit more descriptive? Lukacs suggested 'workspace_root'.

I'm happy to augment this string, but I'm not sure where you're trying to get to here (or specifically how 'workspace_root' fits in). Mind commenting with an example, or giving me a pointer to one of these?

Per directory:
Cleaning [workspace|build|output] directory < asynchronously... if async>
Once for all directories:
Cleaning [workspace|build|output] director{y/ies}< asynchronously... if async>

tbh the inclusion of 'workspace' or 'workspace_root' in the output here for some may indicate that their actual source workspace is in jeopardy, though perhaps not anyone who expects something reasonable out of a build system.

Ok. How about "Output tree"? In the expunge case, we write "Output base", which also includes a bunch of other things. I suppose the distinction is a bit subtle, but I don't see any good way to explain it with two words.

ulfjack

Thanks for addressing the previous comments. There are two more small things that we discovered in the import process.

laszlocsomor · 2016-12-07T12:12:11Z

@ulfjack : Is there a comment for those issues, or is anyone addressing them right now? Context: I'm sheriff today and am trying to assess the status of this PR.

An asynchronous clean without a complete purge and daemon shutdown is particularly useful when the daemon takes an inordinate amount of time to read output files for md5sums. Incurs a garbage collector invocation after cache cleanup in the interest of correct usage reporting.

laszlocsomor · 2016-12-08T10:07:15Z

Thanks! Importing.

laszlocsomor · 2016-12-08T10:07:42Z

@iirina : FYI, I can handle importing this PR

An asynchronous clean without a complete purge and daemon shutdown is particularly useful when the daemon takes an inordinate amount of time to read output files for md5sums. Incurs a garbage collector invocation after cache cleanup in the interest of correct usage reporting. Closes bazelbuild#2053. -- Reviewed-on: bazelbuild#2053 PiperOrigin-RevId: 141437418 MOS_MIGRATED_REVID=141437418

Basically a refactor of #2053, which separated the concepts of async and expunge but kept them intertwined at the option level. This was confusing to a number of users. The standard interface is to use one of --expunge, --async, or --expunge_async. --clean_style was more verbose and added no value, so can be removed. The contents of actuallyClean() could use some ... actual cleaning. This CL just changes the options, removing some of the artificial option-related complexity. RELNOTES[INC]: --clean_style is no longer an option. PiperOrigin-RevId: 177843049

Basically a refactor of bazelbuild/bazel#2053, which separated the concepts of async and expunge but kept them intertwined at the option level. This was confusing to a number of users. The standard interface is to use one of --expunge, --async, or --expunge_async. --clean_style was more verbose and added no value, so can be removed. The contents of actuallyClean() could use some ... actual cleaning. This CL just changes the options, removing some of the artificial option-related complexity. RELNOTES[INC]: --clean_style is no longer an option. PiperOrigin-RevId: 177843049

An asynchronous clean without a complete purge and daemon shutdown is particularly useful when the daemon takes an inordinate amount of time to read output files for md5sums. Incurs a garbage collector invocation after cache cleanup in the interest of correct usage reporting. Closes #2053. -- Reviewed-on: bazelbuild/bazel#2053 PiperOrigin-RevId: 141437418 MOS_MIGRATED_REVID=141437418

googlebot added the cla: no label Nov 7, 2016

damienmg suggested changes Dec 5, 2016

View reviewed changes

damienmg self-assigned this Dec 5, 2016

googlebot added cla: yes and removed cla: no labels Dec 6, 2016

werkt force-pushed the clean_async branch from 2afb45a to d27dd41 Compare December 6, 2016 19:56

damienmg assigned ulfjack and unassigned damienmg Dec 6, 2016

damienmg approved these changes Dec 6, 2016

View reviewed changes

ulfjack reviewed Dec 6, 2016

View reviewed changes

ulfjack approved these changes Dec 6, 2016

View reviewed changes

ulfjack mentioned this pull request Dec 6, 2016

Add test for bazel clean, incl. --async #2196

Closed

werkt force-pushed the clean_async branch from d27dd41 to 29515f3 Compare December 6, 2016 20:43

ulfjack reviewed Dec 6, 2016

View reviewed changes

ulfjack approved these changes Dec 6, 2016

View reviewed changes

werkt force-pushed the clean_async branch from 29515f3 to 12ca294 Compare December 6, 2016 22:34

ulfjack approved these changes Dec 7, 2016

View reviewed changes

werkt force-pushed the clean_async branch from 12ca294 to 239aec8 Compare December 7, 2016 21:05

bazel-io closed this in c75aaeb Dec 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --async clean independent of expunge #2053

Add --async clean independent of expunge #2053

werkt commented Nov 7, 2016

bazel-io commented Nov 7, 2016

googlebot commented Nov 7, 2016

damienmg Dec 5, 2016

werkt Dec 6, 2016

damienmg Dec 5, 2016

damienmg Dec 5, 2016

damienmg Dec 5, 2016

damienmg commented Dec 5, 2016

damienmg commented Dec 5, 2016

werkt commented Dec 6, 2016

damienmg commented Dec 6, 2016

ulfjack Dec 6, 2016 •

edited

Loading

ulfjack Dec 6, 2016

ulfjack left a comment

werkt commented Dec 6, 2016

ulfjack Dec 6, 2016

ulfjack Dec 6, 2016

werkt Dec 6, 2016

ulfjack Dec 7, 2016

ulfjack left a comment

laszlocsomor commented Dec 7, 2016 •

edited

Loading

laszlocsomor commented Dec 8, 2016

laszlocsomor commented Dec 8, 2016

Add --async clean independent of expunge #2053

Add --async clean independent of expunge #2053

Conversation

werkt commented Nov 7, 2016

bazel-io commented Nov 7, 2016

googlebot commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damienmg commented Dec 5, 2016

damienmg commented Dec 5, 2016

werkt commented Dec 6, 2016

damienmg commented Dec 6, 2016

ulfjack Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulfjack left a comment

Choose a reason for hiding this comment

werkt commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulfjack left a comment

Choose a reason for hiding this comment

laszlocsomor commented Dec 7, 2016 • edited Loading

laszlocsomor commented Dec 8, 2016

laszlocsomor commented Dec 8, 2016

ulfjack Dec 6, 2016 •

edited

Loading

laszlocsomor commented Dec 7, 2016 •

edited

Loading