Implement combined cache in RemoteCache.

Previously, combined cache is implemented by `DiskAndRemoteCacheCliet` which is hidden behind the interface `RemoteCacheClient`. This introduced a lot of issues because disk cache is really not a variant of remote cache and we need to handle them differently.

This CL moves the logic from `DiskAndRemoteCacheCliet` to `RemoteCache` so that the code is aware of two caches and:

1. `casUploadCache` is only applied to remote cache.
2. Every functions in `RemoteExecutionCache` only operates remote cache.

The class `RemoteCache` should really be renamed to something else because it doesn't only have remote cache. But this should be a follow-up CL.

Fixes bazelbuild#20962.
Closes bazelbuild#21213.

It's also releated to bazelbuild#20296, but I would like the add the mentioned integration test before close it.

PiperOrigin-RevId: 671740437
Change-Id: Ia55ef879d313d111874b4f8376f66a7d1fceebf7

Author: coeuvre

src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java

@@ -338,7 +338,7 @@ private Single<String> getRemoteServerInstanceName(RemoteCache remoteCache) {
       return Single.just(remoteBytestreamUriPrefix);
     }
 
-    return toSingle(remoteCache.cacheProtocol::getAuthority, directExecutor())
+    return toSingle(remoteCache::getRemoteAuthority, directExecutor())
         .map(
             a -> {
               if (!Strings.isNullOrEmpty(remoteInstanceName)) {

src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java

@@ -20,7 +20,6 @@
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions;
 import com.google.devtools.build.lib.remote.common.RemoteCacheClient;
-import com.google.devtools.build.lib.remote.disk.DiskAndRemoteCacheClient;
 import com.google.devtools.build.lib.remote.disk.DiskCacheClient;
 import com.google.devtools.build.lib.remote.http.HttpCacheClient;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
@@ -40,21 +39,11 @@ public final class RemoteCacheClientFactory {
 
   private RemoteCacheClientFactory() {}
 
-  public static RemoteCacheClient createDiskAndRemoteClient(
-      Path workingDirectory,
-      RemoteOptions options,
-      DigestUtil digestUtil,
-      ExecutorService executorService,
-      boolean remoteVerifyDownloads,
-      RemoteCacheClient remoteCacheClient)
-      throws IOException {
-    DiskCacheClient diskCacheClient =
-        createDiskCache(
-            workingDirectory, options, digestUtil, executorService, remoteVerifyDownloads);
-    return new DiskAndRemoteCacheClient(diskCacheClient, remoteCacheClient);
-  }
+  /** A record that hold both a {@link RemoteCacheClient} and {@link DiskCacheClient}. */
+  public record CombinedCacheClient(
+      @Nullable RemoteCacheClient remoteCacheClient, @Nullable DiskCacheClient diskCacheClient) {}
 
-  public static RemoteCacheClient create(
+  public static CombinedCacheClient create(
       RemoteOptions options,
       @Nullable Credentials creds,
       AuthAndTLSOptions authAndTlsOptions,
@@ -64,26 +53,26 @@ public static RemoteCacheClient create(
       RemoteRetrier retrier)
       throws IOException {
     Preconditions.checkNotNull(workingDirectory, "workingDirectory");
-    if (isHttpCache(options) && isDiskCache(options)) {
-      return createDiskAndHttpCache(
-          workingDirectory,
-          options,
-          creds,
-          authAndTlsOptions,
-          digestUtil,
-          executorService,
-          retrier);
-    }
+    RemoteCacheClient httpCacheClient = null;
+    DiskCacheClient diskCacheClient = null;
     if (isHttpCache(options)) {
-      return createHttp(options, creds, authAndTlsOptions, digestUtil, retrier);
+      httpCacheClient = createHttp(options, creds, authAndTlsOptions, digestUtil, retrier);
     }
     if (isDiskCache(options)) {
-      return createDiskCache(
-          workingDirectory, options, digestUtil, executorService, options.remoteVerifyDownloads);
+      diskCacheClient =
+          createDiskCache(
+              workingDirectory,
+              options,
+              digestUtil,
+              executorService,
+              options.remoteVerifyDownloads);
+    }
+    if (httpCacheClient == null && diskCacheClient == null) {
+      throw new IllegalArgumentException(
+          "Unrecognized RemoteOptions configuration: remote Http cache URL and/or local disk cache"
+              + " options expected.");
     }
-    throw new IllegalArgumentException(
-        "Unrecognized RemoteOptions configuration: remote Http cache URL and/or local disk cache"
-            + " options expected.");
+    return new CombinedCacheClient(httpCacheClient, diskCacheClient);
   }
 
   public static boolean isRemoteCacheOptions(RemoteOptions options) {
@@ -137,7 +126,7 @@ private static RemoteCacheClient createHttp(
     }
   }
 
-  private static DiskCacheClient createDiskCache(
+  public static DiskCacheClient createDiskCache(
       Path workingDirectory,
       RemoteOptions options,
       DigestUtil digestUtil,
@@ -149,25 +138,6 @@ private static DiskCacheClient createDiskCache(
         cacheDir, options.diskCacheMaxSizeBytes, digestUtil, executorService, verifyDownloads);
   }
 
-  private static RemoteCacheClient createDiskAndHttpCache(
-      Path workingDirectory,
-      RemoteOptions options,
-      Credentials cred,
-      AuthAndTLSOptions authAndTlsOptions,
-      DigestUtil digestUtil,
-      ExecutorService executorService,
-      RemoteRetrier retrier)
-      throws IOException {
-    RemoteCacheClient httpCache = createHttp(options, cred, authAndTlsOptions, digestUtil, retrier);
-    return createDiskAndRemoteClient(
-        workingDirectory,
-        options,
-        digestUtil,
-        executorService,
-        options.remoteVerifyDownloads,
-        httpCache);
-  }
-
   public static boolean isDiskCache(RemoteOptions options) {
     return options.diskCache != null && !options.diskCache.isEmpty();
   }

src/main/java/com/google/devtools/build/lib/remote/RemoteCacheClientFactory.java

@@ -40,6 +40,7 @@
 import com.google.devtools.build.lib.remote.common.LostInputsEvent;
 import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
 import com.google.devtools.build.lib.remote.common.RemoteCacheClient;
+import com.google.devtools.build.lib.remote.disk.DiskCacheClient;
 import com.google.devtools.build.lib.remote.merkletree.MerkleTree;
 import com.google.devtools.build.lib.remote.merkletree.MerkleTree.PathOrBytes;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
@@ -61,6 +62,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicReference;
+import javax.annotation.Nullable;
 
 /** A {@link RemoteCache} with additional functionality needed for remote execution. */
 public class RemoteExecutionCache extends RemoteCache {
@@ -99,8 +101,11 @@ public boolean isRemote(RemoteActionExecutionContext context, Path path)
       };
 
   public RemoteExecutionCache(
-      RemoteCacheClient protocolImpl, RemoteOptions options, DigestUtil digestUtil) {
-    super(protocolImpl, options, digestUtil);
+      RemoteCacheClient remoteCacheClient,
+      @Nullable DiskCacheClient diskCacheClient,
+      RemoteOptions options,
+      DigestUtil digestUtil) {
+    super(checkNotNull(remoteCacheClient), diskCacheClient, options, digestUtil);
   }
 
   @VisibleForTesting
@@ -175,13 +180,13 @@ private ListenableFuture<Void> uploadBlob(
       Reporter reporter) {
     Directory node = merkleTree.getDirectoryByDigest(digest);
     if (node != null) {
-      return cacheProtocol.uploadBlob(context, digest, node.toByteString());
+      return remoteCacheClient.uploadBlob(context, digest, node.toByteString());
     }
 
     PathOrBytes file = merkleTree.getFileByDigest(digest);
     if (file != null) {
       if (file.getBytes() != null) {
-        return cacheProtocol.uploadBlob(context, digest, file.getBytes());
+        return remoteCacheClient.uploadBlob(context, digest, file.getBytes());
       }
 
       var path = checkNotNull(file.getPath());
@@ -196,12 +201,12 @@ private ListenableFuture<Void> uploadBlob(
       } catch (IOException e) {
         return immediateFailedFuture(e);
       }
-      return cacheProtocol.uploadFile(context, digest, path);
+      return remoteCacheClient.uploadFile(context, digest, path);
     }
 
     Message message = additionalInputs.get(digest);
     if (message != null) {
-      return cacheProtocol.uploadBlob(context, digest, message.toByteString());
+      return remoteCacheClient.uploadBlob(context, digest, message.toByteString());
     }
 
     return immediateFailedFuture(
@@ -315,7 +320,8 @@ private Single<List<UploadTask>> findMissingBlobs(
                                   if (digestsToQuery.isEmpty()) {
                                     return immediateFuture(ImmutableSet.of());
                                   }
-                                  return findMissingDigests(context, digestsToQuery);
+                                  return remoteCacheClient.findMissingDigests(
+                                      context, digestsToQuery);
                                 },
                                 directExecutor())
                             .map(

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionCache.java

@@ -16,7 +16,6 @@
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
-import static com.google.common.base.Strings.isNullOrEmpty;
 import static com.google.common.collect.ImmutableMap.toImmutableMap;
 import static com.google.common.util.concurrent.Futures.immediateFailedFuture;
 import static com.google.common.util.concurrent.Futures.immediateFuture;
@@ -286,33 +285,22 @@ private Command buildCommand(
     return command.setWorkingDirectory(remotePathResolver.getWorkingDirectory()).build();
   }
 
-  private static boolean useRemoteCache(RemoteOptions options) {
-    return !isNullOrEmpty(options.remoteCache) || !isNullOrEmpty(options.remoteExecutor);
+  private boolean useRemoteCache() {
+    return remoteCache != null && remoteCache.hasRemoteCache();
   }
 
-  private static boolean useDiskCache(RemoteOptions options) {
-    return options.diskCache != null && !options.diskCache.isEmpty();
+  private boolean useDiskCache() {
+    return remoteCache != null && remoteCache.hasDiskCache();
   }
 
   public CachePolicy getReadCachePolicy(Spawn spawn) {
     if (remoteCache == null) {
       return CachePolicy.NO_CACHE;
     }
 
-    boolean allowDiskCache = false;
-    boolean allowRemoteCache = false;
-
-    if (useRemoteCache(remoteOptions)) {
-      allowRemoteCache = remoteOptions.remoteAcceptCached && Spawns.mayBeCachedRemotely(spawn);
-      if (useDiskCache(remoteOptions)) {
-        // Combined cache. Disk cache is treated as local cache. Actions which are tagged with
-        // `no-remote-cache` can still hit the disk cache.
-        allowDiskCache = Spawns.mayBeCached(spawn);
-      }
-    } else {
-      // Disk cache only
-      allowDiskCache = Spawns.mayBeCached(spawn);
-    }
+    boolean allowRemoteCache =
+        useRemoteCache() && remoteOptions.remoteAcceptCached && Spawns.mayBeCachedRemotely(spawn);
+    boolean allowDiskCache = useDiskCache() && Spawns.mayBeCached(spawn);
 
     return CachePolicy.create(allowRemoteCache, allowDiskCache);
   }
@@ -322,22 +310,11 @@ public CachePolicy getWriteCachePolicy(Spawn spawn) {
       return CachePolicy.NO_CACHE;
     }
 
-    boolean allowDiskCache = false;
-    boolean allowRemoteCache = false;
-
-    if (useRemoteCache(remoteOptions)) {
-      allowRemoteCache =
-          shouldUploadLocalResultsToRemoteCache(remoteOptions, spawn.getExecutionInfo())
-              && remoteCache.actionCacheSupportsUpdate();
-      if (useDiskCache(remoteOptions)) {
-        // Combined cache. Treat the disk cache part as local cache. Actions which are tagged with
-        // `no-remote-cache` can still hit the disk cache.
-        allowDiskCache = Spawns.mayBeCached(spawn);
-      }
-    } else {
-      // Disk cache only
-      allowDiskCache = Spawns.mayBeCached(spawn);
-    }
+    boolean allowRemoteCache =
+        useRemoteCache()
+            && shouldUploadLocalResultsToRemoteCache(remoteOptions, spawn.getExecutionInfo())
+            && remoteCache.remoteActionCacheSupportsUpdate();
+    boolean allowDiskCache = useDiskCache() && Spawns.mayBeCached(spawn);
 
     return CachePolicy.create(allowRemoteCache, allowDiskCache);
   }
@@ -1310,7 +1287,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
       // When downloading outputs from just remotely executed action, the action result comes from
       // Execution response which means, if disk cache is enabled, action result hasn't been
       // uploaded to it. Upload action result to disk cache here so next build could hit it.
-      if (useDiskCache(remoteOptions) && result.executeResponse != null) {
+      if (useDiskCache() && result.executeResponse != null) {
         getFromFuture(
             remoteCache.uploadActionResult(
                 context.withWriteCachePolicy(CachePolicy.DISK_CACHE_ONLY),
@@ -1583,7 +1560,7 @@ private Single<UploadManifest> buildUploadManifestAsync(
 
             return UploadManifest.create(
                 remoteOptions,
-                remoteCache.getCacheCapabilities(),
+                remoteCache.getRemoteCacheCapabilities(),
                 digestUtil,
                 action.getRemotePathResolver(),
                 action.getActionKey(),