Refactor combined cache.

Instead of guessing when to use remote/disk part, combined cache now uses read/write cache policy provided by the context. The call sites can change the policy based on the requirements. Fixes #15934. But unfortunately, I am not able to add an integration test for it because our own remote worker doesn't support the asset API.
bazelbuild · Aug 3, 2022 · a28746c · a28746c
1 parent 6e4bf7d
commit a28746c
Show file tree

Hide file tree

Showing 10 changed files with 232 additions and 251 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java
@@ -30,7 +30,7 @@
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
-import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext.Step;
+import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext.CachePolicy;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
 import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -277,8 +277,9 @@ private Single<PathConverter> upload(Set<Path> files) {
 
     RequestMetadata metadata =
         TracingMetadataUtils.buildMetadata(buildRequestId, commandId, "bes-upload", null);
-    RemoteActionExecutionContext context = RemoteActionExecutionContext.create(metadata);
-    context.setStep(Step.UPLOAD_BES_FILES);
+    RemoteActionExecutionContext context =
+        RemoteActionExecutionContext.create(metadata)
+            .withWriteCachePolicy(CachePolicy.REMOTE_CACHE_ONLY);
 
     return Single.using(
         remoteCache::retain,

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteAction.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteAction.java
@@ -131,4 +131,17 @@ public SortedMap<PathFragment, ActionInput> getInputMap()
   public NetworkTime getNetworkTime() {
     return remoteActionExecutionContext.getNetworkTime();
   }
+
+  /** Returns {@code true} if the {@code spawn} should accept cached results from remote cache. */
+  public boolean shouldAcceptCachedResult() {
+    return remoteActionExecutionContext.getReadCachePolicy().allowAnyCache();
+  }
+
+  /**
+   * Returns {@code true} if the local results of the {@code spawn} should be uploaded to remote
+   * cache.
+   */
+  public boolean shouldUploadLocalResults() {
+    return remoteActionExecutionContext.getWriteCachePolicy().allowAnyCache();
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCacheClientFactory.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCacheClientFactory.java
@@ -45,12 +45,11 @@ public static RemoteCacheClient createDiskAndRemoteClient(
       PathFragment diskCachePath,
       boolean remoteVerifyDownloads,
       DigestUtil digestUtil,
-      RemoteCacheClient remoteCacheClient,
-      RemoteOptions options)
+      RemoteCacheClient remoteCacheClient)
       throws IOException {
     DiskCacheClient diskCacheClient =
         createDiskCache(workingDirectory, diskCachePath, remoteVerifyDownloads, digestUtil);
-    return new DiskAndRemoteCacheClient(diskCacheClient, remoteCacheClient, options);
+    return new DiskAndRemoteCacheClient(diskCacheClient, remoteCacheClient);
   }
 
   public static RemoteCacheClient create(
@@ -155,12 +154,7 @@ private static RemoteCacheClient createDiskAndHttpCache(
 
     RemoteCacheClient httpCache = createHttp(options, cred, authAndTlsOptions, digestUtil);
     return createDiskAndRemoteClient(
-        workingDirectory,
-        diskCachePath,
-        options.remoteVerifyDownloads,
-        digestUtil,
-        httpCache,
-        options);
+        workingDirectory, diskCachePath, options.remoteVerifyDownloads, digestUtil, httpCache);
   }
 
   public static boolean isDiskCache(RemoteOptions options) {

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java
@@ -555,8 +555,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
                   remoteOptions.diskCache,
                   remoteOptions.remoteVerifyDownloads,
                   digestUtil,
-                  cacheClient,
-                  remoteOptions);
+                  cacheClient);
         } catch (IOException e) {
           handleInitFailure(env, e, Code.CACHE_INIT_FAILURE);
           return;
@@ -614,8 +613,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
                   remoteOptions.diskCache,
                   remoteOptions.remoteVerifyDownloads,
                   digestUtil,
-                  cacheClient,
-                  remoteOptions);
+                  cacheClient);
         } catch (IOException e) {
           handleInitFailure(env, e, Code.CACHE_INIT_FAILURE);
           return;

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
@@ -82,15 +82,15 @@ RemoteExecutionService getRemoteExecutionService() {
   @Override
   public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
       throws InterruptedException, IOException, ExecException, ForbiddenActionInputException {
-    boolean shouldAcceptCachedResult = remoteExecutionService.shouldAcceptCachedResult(spawn);
-    boolean shouldUploadLocalResults = remoteExecutionService.shouldUploadLocalResults(spawn);
+    RemoteAction action = remoteExecutionService.buildRemoteAction(spawn, context);
+    boolean shouldAcceptCachedResult = action.shouldAcceptCachedResult();
+    boolean shouldUploadLocalResults = action.shouldUploadLocalResults();
     if (!shouldAcceptCachedResult && !shouldUploadLocalResults) {
       return SpawnCache.NO_RESULT_NO_STORE;
     }
 
     Stopwatch totalTime = Stopwatch.createStarted();
 
-    RemoteAction action = remoteExecutionService.buildRemoteAction(spawn, context);
     SpawnMetrics.Builder spawnMetrics =
         SpawnMetrics.Builder.forRemoteExec()
             .setInputBytes(action.getInputBytes())

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
@@ -183,10 +183,10 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
         "Spawn can't be executed remotely. This is a bug.");
 
     Stopwatch totalTime = Stopwatch.createStarted();
-    boolean uploadLocalResults = remoteExecutionService.shouldUploadLocalResults(spawn);
-    boolean acceptCachedResult = remoteExecutionService.shouldAcceptCachedResult(spawn);
-
     RemoteAction action = remoteExecutionService.buildRemoteAction(spawn, context);
+    boolean uploadLocalResults = action.shouldUploadLocalResults();
+    boolean acceptCachedResult = action.shouldAcceptCachedResult();
+
     SpawnMetrics.Builder spawnMetrics =
         SpawnMetrics.Builder.forRemoteExec()
             .setInputBytes(action.getInputBytes())

diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java
@@ -13,48 +13,88 @@
 // limitations under the License.
 package com.google.devtools.build.lib.remote.common;
 
-import build.bazel.remote.execution.v2.ExecuteResponse;
 import build.bazel.remote.execution.v2.RequestMetadata;
 import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
 import com.google.devtools.build.lib.actions.Spawn;
 import javax.annotation.Nullable;
 
 /** A context that provide remote execution related information for executing an action remotely. */
 public class RemoteActionExecutionContext {
-  /** The current step of the context. */
-  public enum Step {
-    INIT,
-    CHECK_ACTION_CACHE,
-    UPLOAD_INPUTS,
-    EXECUTE_REMOTELY,
-    UPLOAD_OUTPUTS,
-    DOWNLOAD_OUTPUTS,
-    UPLOAD_BES_FILES,
+  public enum CachePolicy {
+    NO_CACHE,
+    REMOTE_CACHE_ONLY,
+    DISK_CACHE_ONLY,
+    ANY_CACHE;
+
+    public static CachePolicy create(boolean allowRemoteCache, boolean allowDiskCache) {
+      if (allowRemoteCache && allowDiskCache) {
+        return ANY_CACHE;
+      } else if (allowRemoteCache) {
+        return REMOTE_CACHE_ONLY;
+      } else if (allowDiskCache) {
+        return DISK_CACHE_ONLY;
+      } else {
+        return NO_CACHE;
+      }
+    }
+
+    public boolean allowAnyCache() {
+      return this != NO_CACHE;
+    }
+
+    public boolean allowRemoteCache() {
+      return this == REMOTE_CACHE_ONLY || this == ANY_CACHE;
+    }
+
+    public boolean allowDiskCache() {
+      return this == DISK_CACHE_ONLY || this == ANY_CACHE;
+    }
+
+    public CachePolicy addRemoteCache() {
+      if (this == DISK_CACHE_ONLY || this == ANY_CACHE) {
+        return ANY_CACHE;
+      }
+
+      return REMOTE_CACHE_ONLY;
+    }
   }
 
   @Nullable private final Spawn spawn;
   private final RequestMetadata requestMetadata;
   private final NetworkTime networkTime;
-
-  @Nullable private ExecuteResponse executeResponse;
-  private Step step;
+  private final CachePolicy writeCachePolicy;
+  private final CachePolicy readCachePolicy;
 
   private RemoteActionExecutionContext(
       @Nullable Spawn spawn, RequestMetadata requestMetadata, NetworkTime networkTime) {
     this.spawn = spawn;
     this.requestMetadata = requestMetadata;
     this.networkTime = networkTime;
-    this.step = Step.INIT;
+    this.writeCachePolicy = CachePolicy.ANY_CACHE;
+    this.readCachePolicy = CachePolicy.ANY_CACHE;
   }
 
-  /** Returns current {@link Step} of the context. */
-  public Step getStep() {
-    return step;
+  private RemoteActionExecutionContext(
+      @Nullable Spawn spawn,
+      RequestMetadata requestMetadata,
+      NetworkTime networkTime,
+      CachePolicy writeCachePolicy,
+      CachePolicy readCachePolicy) {
+    this.spawn = spawn;
+    this.requestMetadata = requestMetadata;
+    this.networkTime = networkTime;
+    this.writeCachePolicy = writeCachePolicy;
+    this.readCachePolicy = readCachePolicy;
+  }
+
+  public RemoteActionExecutionContext withWriteCachePolicy(CachePolicy writeCachePolicy) {
+    return new RemoteActionExecutionContext(
+        spawn, requestMetadata, networkTime, writeCachePolicy, readCachePolicy);
   }
 
-  /** Sets current {@link Step} of the context. */
-  public void setStep(Step step) {
-    this.step = step;
+  public RemoteActionExecutionContext withReadCachePolicy(CachePolicy readCachePolicy) {
+    return new RemoteActionExecutionContext(
+        spawn, requestMetadata, networkTime, writeCachePolicy, readCachePolicy);
   }
 
   /** Returns the {@link Spawn} of the action being executed or {@code null}. */
@@ -86,13 +126,12 @@ public ActionExecutionMetadata getSpawnOwner() {
     return spawn.getResourceOwner();
   }
 
-  public void setExecuteResponse(@Nullable ExecuteResponse executeResponse) {
-    this.executeResponse = executeResponse;
+  public CachePolicy getWriteCachePolicy() {
+    return writeCachePolicy;
   }
 
-  @Nullable
-  public ExecuteResponse getExecuteResponse() {
-    return executeResponse;
+  public CachePolicy getReadCachePolicy() {
+    return readCachePolicy;
   }
 
   /** Creates a {@link RemoteActionExecutionContext} with given {@link RequestMetadata}. */
@@ -108,4 +147,13 @@ public static RemoteActionExecutionContext create(
       @Nullable Spawn spawn, RequestMetadata metadata) {
     return new RemoteActionExecutionContext(spawn, metadata, new NetworkTime());
   }
+
+  public static RemoteActionExecutionContext create(
+      @Nullable Spawn spawn,
+      RequestMetadata requestMetadata,
+      CachePolicy writeCachePolicy,
+      CachePolicy readCachePolicy) {
+    return new RemoteActionExecutionContext(
+        spawn, requestMetadata, new NetworkTime(), writeCachePolicy, readCachePolicy);
+  }
 }