Revert "Remote: Report checking cache status before the action is scheduled to run remotely."

This reverts commit 97d7b4c.

Author: larsrc-google

src/main/java/com/google/devtools/build/lib/actions/CachingActionEvent.java

@@ -28,7 +28,9 @@
 import com.google.devtools.build.lib.actions.LostInputsActionExecutionException;
 import com.google.devtools.build.lib.actions.LostInputsExecException;
 import com.google.devtools.build.lib.actions.MetadataProvider;
+import com.google.devtools.build.lib.actions.RunningActionEvent;
 import com.google.devtools.build.lib.actions.SandboxedSpawnStrategy;
+import com.google.devtools.build.lib.actions.SchedulingActionEvent;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnExecutedEvent;
 import com.google.devtools.build.lib.actions.SpawnResult;
@@ -306,7 +308,7 @@ public SortedMap<PathFragment, ActionInput> getInputMapping(PathFragment baseDir
     }
 
     @Override
-    public void report(ProgressStatus progress) {
+    public void report(ProgressStatus state, String name) {
       ActionExecutionMetadata action = spawn.getResourceOwner();
       if (action.getOwner() == null) {
         return;
@@ -320,7 +322,17 @@ public void report(ProgressStatus progress) {
 
       // TODO(ulfjack): We should report more details to the UI.
       ExtendedEventHandler eventHandler = actionExecutionContext.getEventHandler();
-      progress.postTo(eventHandler, action);
+      switch (state) {
+        case EXECUTING:
+        case CHECKING_CACHE:
+          eventHandler.post(new RunningActionEvent(action, name));
+          break;
+        case SCHEDULING:
+          eventHandler.post(new SchedulingActionEvent(action, name));
+          break;
+        default:
+          break;
+      }
     }
 
     @Override

src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java

@@ -264,21 +264,14 @@ java_library(
 
 java_library(
     name = "spawn_runner",
-    srcs = [
-        "SpawnCheckingCacheEvent.java",
-        "SpawnExecutingEvent.java",
-        "SpawnRunner.java",
-        "SpawnSchedulingEvent.java",
-    ],
+    srcs = ["SpawnRunner.java"],
     deps = [
         ":tree_deleter",
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
-        "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/util/io",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
-        "//third_party:auto_value",
         "//third_party:jsr305",
     ],
 )

src/main/java/com/google/devtools/build/lib/exec/BUILD

@@ -14,7 +14,6 @@
 package com.google.devtools.build.lib.exec;
 
 import com.google.devtools.build.lib.actions.ActionContext;
-import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
 import com.google.devtools.build.lib.actions.ArtifactPathResolver;
@@ -25,7 +24,6 @@
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.actions.cache.MetadataInjector;
-import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -105,9 +103,25 @@ public interface SpawnRunner {
    * <p>{@link SpawnRunner} implementations should post a progress status before any potentially
    * long-running operation.
    */
-  interface ProgressStatus {
-    /** Post this progress event to the given {@link ExtendedEventHandler}. */
-    void postTo(ExtendedEventHandler eventHandler, ActionExecutionMetadata action);
+  enum ProgressStatus {
+    /** Spawn is waiting for local or remote resources to become available. */
+    SCHEDULING,
+
+    /** The {@link SpawnRunner} is looking for a cache hit. */
+    CHECKING_CACHE,
+
+    /**
+     * Resources are acquired, and there was probably no cache hit. This MUST be posted before
+     * attempting to execute the subprocess.
+     *
+     * <p>Caching {@link SpawnRunner} implementations should only post this after a failed cache
+     * lookup, but may post this if cache lookup and execution happen within the same step, e.g. as
+     * part of a single RPC call with no mechanism to report cache misses.
+     */
+    EXECUTING,
+
+    /** Downloading outputs from a remote machine. */
+    DOWNLOADING
   }
 
   /**
@@ -199,7 +213,7 @@ SortedMap<PathFragment, ActionInput> getInputMapping(PathFragment baseDirectory)
         throws IOException;
 
     /** Reports a progress update to the Spawn strategy. */
-    void report(ProgressStatus progress);
+    void report(ProgressStatus state, String name);
 
     /**
      * Returns a {@link MetadataInjector} that allows a caller to inject metadata about spawn

src/main/java/com/google/devtools/build/lib/exec/SpawnCheckingCacheEvent.java

@@ -38,9 +38,7 @@
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.exec.BinTools;
 import com.google.devtools.build.lib.exec.RunfilesTreeUpdater;
-import com.google.devtools.build.lib.exec.SpawnExecutingEvent;
 import com.google.devtools.build.lib.exec.SpawnRunner;
-import com.google.devtools.build.lib.exec.SpawnSchedulingEvent;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.ProfilerTask;
 import com.google.devtools.build.lib.profiler.SilentCloseable;
@@ -133,10 +131,10 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
         Profiler.instance()
             .profile(ProfilerTask.LOCAL_EXECUTION, spawn.getResourceOwner().getMnemonic())) {
       ActionExecutionMetadata owner = spawn.getResourceOwner();
-      context.report(SpawnSchedulingEvent.create(getName()));
+      context.report(ProgressStatus.SCHEDULING, getName());
       try (ResourceHandle handle =
           resourceManager.acquireResources(owner, spawn.getLocalResources())) {
-        context.report(SpawnExecutingEvent.create(getName()));
+        context.report(ProgressStatus.EXECUTING, getName());
         if (!localExecutionOptions.localLockfreeOutput) {
           context.lockOutputFiles();
         }

src/main/java/com/google/devtools/build/lib/exec/SpawnExecutingEvent.java

@@ -31,8 +31,7 @@
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.exec.SpawnCache;
-import com.google.devtools.build.lib.exec.SpawnCheckingCacheEvent;
-import com.google.devtools.build.lib.exec.SpawnExecutingEvent;
+import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus;
 import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.ProfilerTask;
@@ -54,12 +53,6 @@
 @ThreadSafe // If the RemoteActionCache implementation is thread-safe.
 final class RemoteSpawnCache implements SpawnCache {
 
-  private static final SpawnCheckingCacheEvent SPAWN_CHECKING_CACHE_EVENT =
-      SpawnCheckingCacheEvent.create("remote-cache");
-
-  private static final SpawnExecutingEvent SPAWN_EXECUTING_EVENT =
-      SpawnExecutingEvent.create("remote-cache");
-
   private final Path execRoot;
   private final RemoteOptions options;
   private final boolean verboseFailures;
@@ -103,7 +96,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
     Profiler prof = Profiler.instance();
     if (options.remoteAcceptCached
         || (options.incompatibleRemoteResultsIgnoreDisk && useDiskCache(options))) {
-      context.report(SPAWN_CHECKING_CACHE_EVENT);
+      context.report(ProgressStatus.CHECKING_CACHE, "remote-cache");
       // Metadata will be available in context.current() until we detach.
       // This is done via a thread-local variable.
       try {
@@ -157,8 +150,6 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
       }
     }
 
-    context.report(SPAWN_EXECUTING_EVENT);
-
     context.prefetchInputs();
 
     if (options.remoteUploadLocalResults

src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java

@@ -44,10 +44,7 @@
 import com.google.devtools.build.lib.exec.AbstractSpawnStrategy;
 import com.google.devtools.build.lib.exec.ExecutionOptions;
 import com.google.devtools.build.lib.exec.RemoteLocalFallbackRegistry;
-import com.google.devtools.build.lib.exec.SpawnCheckingCacheEvent;
-import com.google.devtools.build.lib.exec.SpawnExecutingEvent;
 import com.google.devtools.build.lib.exec.SpawnRunner;
-import com.google.devtools.build.lib.exec.SpawnSchedulingEvent;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.ProfilerTask;
 import com.google.devtools.build.lib.profiler.SilentCloseable;
@@ -83,15 +80,6 @@
 @ThreadSafe
 public class RemoteSpawnRunner implements SpawnRunner {
 
-  private static final SpawnCheckingCacheEvent SPAWN_CHECKING_CACHE_EVENT =
-      SpawnCheckingCacheEvent.create("remote");
-
-  private static final SpawnSchedulingEvent SPAWN_SCHEDULING_EVENT =
-      SpawnSchedulingEvent.create("remote");
-
-  private static final SpawnExecutingEvent SPAWN_EXECUTING_EVENT =
-      SpawnExecutingEvent.create("remote");
-
   private final Path execRoot;
   private final RemoteOptions remoteOptions;
   private final ExecutionOptions executionOptions;
@@ -154,7 +142,7 @@ public void onNext(Operation o) throws IOException {
     }
 
     public void reportExecuting() {
-      context.report(SPAWN_EXECUTING_EVENT);
+      context.report(ProgressStatus.EXECUTING, getName());
       reportedExecuting = true;
     }
 
@@ -176,6 +164,8 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
     boolean uploadLocalResults = remoteOptions.remoteUploadLocalResults && spawnCacheableRemotely;
     boolean acceptCachedResult = remoteOptions.remoteAcceptCached && spawnCacheableRemotely;
 
+    context.report(ProgressStatus.SCHEDULING, getName());
+
     RemoteAction action = remoteExecutionService.buildRemoteAction(spawn, context);
     SpawnMetrics.Builder spawnMetrics =
         SpawnMetrics.Builder.forRemoteExec()
@@ -188,8 +178,6 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
 
     Profiler prof = Profiler.instance();
     try {
-      context.report(SPAWN_CHECKING_CACHE_EVENT);
-
       // Try to lookup the action in the action cache.
       RemoteActionResult cachedResult;
       try (SilentCloseable c = prof.profile(ProfilerTask.REMOTE_CACHE_CHECK, "check cache hit")) {
@@ -243,8 +231,6 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
                       .minus(action.getNetworkTime().getDuration().minus(networkTimeStart)));
             }
 
-            context.report(SPAWN_SCHEDULING_EVENT);
-
             ExecutingStatusReporter reporter = new ExecutingStatusReporter(context);
             RemoteActionResult result;
             try (SilentCloseable c = prof.profile(REMOTE_EXECUTION, "execute remotely")) {