[6.2.0] Remove actionId from RemoteFileArtifactValue. (#17724)

actionId was added by #11236 to track download requests for input prefetches.

However, according to the REAPI spec, action_id is

> An identifier that ties multiple requests to the same action. For example, multiple requests to the CAS, Action Cache, and Execution API are used in order to compile foo.cc.

The input prefetches are requested by local actions that depend on outputs of remote executed action (which is identified by the action_id). These requests are not part of generating action hence shouldn't be bound with the action_id.

We could include ActionExecutionMetadata of the local action for the requests for the tracking purpose, but that desires another CL.

PiperOrigin-RevId: 510430853
Change-Id: I8d64a4f7033320a394e02a2b11498f09a3d15310

Co-authored-by: Googler <[email protected]>

Author: tjgq

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

@@ -109,15 +109,6 @@ public int getLocationIndex() {
     return 0;
   }
 
-  /**
-   * Remote action source identifier for the file.
-   *
-   * <p>"" indicates that a remote action output was not the source of this artifact.
-   */
-  public String getActionId() {
-    return "";
-  }
-
   /** Returns {@code true} if the file only exists remotely. */
   public boolean isRemote() {
     return false;
@@ -550,35 +541,27 @@ public static class RemoteFileArtifactValue extends FileArtifactValue {
     protected final byte[] digest;
     protected final long size;
     protected final int locationIndex;
-    protected final String actionId;
 
-    private RemoteFileArtifactValue(byte[] digest, long size, int locationIndex, String actionId) {
-      this.digest = Preconditions.checkNotNull(digest, actionId);
+    private RemoteFileArtifactValue(byte[] digest, long size, int locationIndex) {
+      this.digest = Preconditions.checkNotNull(digest);
       this.size = size;
       this.locationIndex = locationIndex;
-      this.actionId = actionId;
-    }
-
-    public static RemoteFileArtifactValue create(
-        byte[] digest, long size, int locationIndex, String actionId) {
-      return new RemoteFileArtifactValue(digest, size, locationIndex, actionId);
     }
 
     public static RemoteFileArtifactValue create(byte[] digest, long size, int locationIndex) {
-      return new RemoteFileArtifactValue(digest, size, locationIndex, /* actionId= */ "");
+      return new RemoteFileArtifactValue(digest, size, locationIndex);
     }
 
     public static RemoteFileArtifactValue create(
         byte[] digest,
         long size,
         int locationIndex,
-        String actionId,
         @Nullable PathFragment materializationExecPath) {
       if (materializationExecPath != null) {
         return new RemoteFileArtifactValueWithMaterializationPath(
-            digest, size, locationIndex, actionId, materializationExecPath);
+            digest, size, locationIndex, materializationExecPath);
       }
-      return new RemoteFileArtifactValue(digest, size, locationIndex, actionId);
+      return new RemoteFileArtifactValue(digest, size, locationIndex);
     }
 
     @Override
@@ -590,13 +573,12 @@ public boolean equals(Object o) {
       RemoteFileArtifactValue that = (RemoteFileArtifactValue) o;
       return Arrays.equals(digest, that.digest)
           && size == that.size
-          && locationIndex == that.locationIndex
-          && Objects.equals(actionId, that.actionId);
+          && locationIndex == that.locationIndex;
     }
 
     @Override
     public int hashCode() {
-      return Objects.hash(Arrays.hashCode(digest), size, locationIndex, actionId);
+      return Objects.hash(Arrays.hashCode(digest), size, locationIndex);
     }
 
     @Override
@@ -619,11 +601,6 @@ public long getSize() {
       return size;
     }
 
-    @Override
-    public String getActionId() {
-      return actionId;
-    }
-
     @Override
     public long getModifiedTime() {
       throw new UnsupportedOperationException(
@@ -651,7 +628,6 @@ public String toString() {
           .add("digest", bytesToString(digest))
           .add("size", size)
           .add("locationIndex", locationIndex)
-          .add("actionId", actionId)
           .toString();
     }
   }
@@ -667,12 +643,8 @@ public static final class RemoteFileArtifactValueWithMaterializationPath
     private final PathFragment materializationExecPath;
 
     private RemoteFileArtifactValueWithMaterializationPath(
-        byte[] digest,
-        long size,
-        int locationIndex,
-        String actionId,
-        PathFragment materializationExecPath) {
-      super(digest, size, locationIndex, actionId);
+        byte[] digest, long size, int locationIndex, PathFragment materializationExecPath) {
+      super(digest, size, locationIndex);
       this.materializationExecPath = Preconditions.checkNotNull(materializationExecPath);
     }
 
@@ -692,14 +664,12 @@ public boolean equals(Object o) {
       return Arrays.equals(digest, that.digest)
           && size == that.size
           && locationIndex == that.locationIndex
-          && Objects.equals(actionId, that.actionId)
           && Objects.equals(materializationExecPath, that.materializationExecPath);
     }
 
     @Override
     public int hashCode() {
-      return Objects.hash(
-          Arrays.hashCode(digest), size, locationIndex, actionId, materializationExecPath);
+      return Objects.hash(Arrays.hashCode(digest), size, locationIndex, materializationExecPath);
     }
 
     @Override
@@ -708,7 +678,6 @@ public String toString() {
           .add("digest", bytesToString(digest))
           .add("size", size)
           .add("locationIndex", locationIndex)
-          .add("actionId", actionId)
           .add("materializationExecPath", materializationExecPath)
           .toString();
     }

src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java

@@ -76,7 +76,7 @@ public class CompactPersistentActionCache implements ActionCache {
 
   private static final int NO_INPUT_DISCOVERY_COUNT = -1;
 
-  private static final int VERSION = 14;
+  private static final int VERSION = 15;
 
   private static final class ActionMap extends PersistentMap<Integer, byte[]> {
     private final Clock clock;
@@ -466,8 +466,6 @@ private static void encodeRemoteMetadata(
 
     VarInt.putVarInt(value.getLocationIndex(), sink);
 
-    VarInt.putVarInt(indexer.getOrCreateIndex(value.getActionId()), sink);
-
     Optional<PathFragment> materializationExecPath = value.getMaterializationExecPath();
     if (materializationExecPath.isPresent()) {
       VarInt.putVarInt(1, sink);
@@ -491,8 +489,6 @@ private static RemoteFileArtifactValue decodeRemoteMetadata(
 
     int locationIndex = VarInt.getVarInt(source);
 
-    String actionId = getStringForIndex(indexer, VarInt.getVarInt(source));
-
     PathFragment materializationExecPath = null;
     int numMaterializationExecPath = VarInt.getVarInt(source);
     if (numMaterializationExecPath > 0) {
@@ -503,8 +499,7 @@ private static RemoteFileArtifactValue decodeRemoteMetadata(
           PathFragment.create(getStringForIndex(indexer, VarInt.getVarInt(source)));
     }
 
-    return RemoteFileArtifactValue.create(
-        digest, size, locationIndex, actionId, materializationExecPath);
+    return RemoteFileArtifactValue.create(digest, size, locationIndex, materializationExecPath);
   }
 
   /** @return action data encoded as a byte[] array. */

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

@@ -115,12 +115,11 @@ public void updateContext(MetadataInjector metadataInjector) {
     this.metadataInjector = metadataInjector;
   }
 
-  void injectRemoteFile(PathFragment path, byte[] digest, long size, String actionId)
-      throws IOException {
+  void injectRemoteFile(PathFragment path, byte[] digest, long size) throws IOException {
     if (!isOutput(path)) {
       return;
     }
-    remoteOutputTree.injectRemoteFile(path, digest, size, actionId);
+    remoteOutputTree.injectRemoteFile(path, digest, size);
   }
 
   void flush() throws IOException {
@@ -207,7 +206,6 @@ private void maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact outpu
               metadata.getDigest(),
               metadata.getSize(),
               metadata.getLocationIndex(),
-              metadata.getActionId(),
               // Avoid a double indirection when the target is already materialized as a symlink.
               metadata.getMaterializationExecPath().orElse(targetPath.relativeTo(execRoot)));
 
@@ -217,10 +215,7 @@ private void maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact outpu
 
   private RemoteFileArtifactValue createRemoteMetadata(RemoteFileInfo remoteFile) {
     return RemoteFileArtifactValue.create(
-        remoteFile.getFastDigest(),
-        remoteFile.getSize(),
-        /* locationIndex= */ 1,
-        remoteFile.getActionId());
+        remoteFile.getFastDigest(), remoteFile.getSize(), /* locationIndex= */ 1);
   }
 
   @Override
@@ -748,8 +743,7 @@ protected FileInfo newFile(Clock clock, PathFragment path) {
       return new RemoteFileInfo(clock);
     }
 
-    void injectRemoteFile(PathFragment path, byte[] digest, long size, String actionId)
-        throws IOException {
+    void injectRemoteFile(PathFragment path, byte[] digest, long size) throws IOException {
       createDirectoryAndParents(path.getParentDirectory());
       InMemoryContentInfo node = getOrCreateWritableInode(path);
       // If a node was already existed and is not a remote file node (i.e. directory or symlink node
@@ -759,7 +753,7 @@ void injectRemoteFile(PathFragment path, byte[] digest, long size, String action
       }
 
       RemoteFileInfo remoteFileInfo = (RemoteFileInfo) node;
-      remoteFileInfo.set(digest, size, actionId);
+      remoteFileInfo.set(digest, size);
     }
 
     @Nullable
@@ -776,16 +770,14 @@ static class RemoteFileInfo extends FileInfo {
 
     private byte[] digest;
     private long size;
-    private String actionId;
 
     RemoteFileInfo(Clock clock) {
       super(clock);
     }
 
-    private void set(byte[] digest, long size, String actionId) {
+    private void set(byte[] digest, long size) {
       this.digest = digest;
       this.size = size;
-      this.actionId = actionId;
     }
 
     @Override
@@ -812,9 +804,5 @@ public byte[] getFastDigest() {
     public long getSize() {
       return size;
     }
-
-    public String getActionId() {
-      return actionId;
-    }
   }
 }

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

@@ -104,7 +104,7 @@ protected ListenableFuture<Void> doDownloadFile(
       throws IOException {
     checkArgument(metadata.isRemote(), "Cannot download file that is not a remote file.");
     RequestMetadata requestMetadata =
-        TracingMetadataUtils.buildMetadata(buildRequestId, commandId, metadata.getActionId(), null);
+        TracingMetadataUtils.buildMetadata(buildRequestId, commandId, "prefetcher", null);
     RemoteActionExecutionContext context = RemoteActionExecutionContext.create(requestMetadata);
 
     Digest digest = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize());

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

@@ -812,7 +812,6 @@ private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata met
     FileSystem actionFileSystem = action.getSpawnExecutionContext().getActionFileSystem();
     checkState(actionFileSystem instanceof RemoteActionFileSystem);
 
-    RemoteActionExecutionContext context = action.getRemoteActionExecutionContext();
     RemoteActionFileSystem remoteActionFileSystem = (RemoteActionFileSystem) actionFileSystem;
 
     for (Map.Entry<Path, DirectoryMetadata> entry : metadata.directories()) {
@@ -827,17 +826,15 @@ private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata met
         remoteActionFileSystem.injectRemoteFile(
             file.path().asFragment(),
             DigestUtil.toBinaryDigest(file.digest()),
-            file.digest().getSizeBytes(),
-            context.getRequestMetadata().getActionId());
+            file.digest().getSizeBytes());
       }
     }
 
     for (FileMetadata file : metadata.files()) {
       remoteActionFileSystem.injectRemoteFile(
           file.path().asFragment(),
           DigestUtil.toBinaryDigest(file.digest()),
-          file.digest().getSizeBytes(),
-          context.getRequestMetadata().getActionId());
+          file.digest().getSizeBytes());
     }
   }
 

src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java

@@ -463,8 +463,7 @@ private RemoteFileArtifactValue createRemoteFileMetadata(String content) {
   private RemoteFileArtifactValue createRemoteFileMetadata(
       String content, @Nullable PathFragment materializationExecPath) {
     byte[] bytes = content.getBytes(UTF_8);
-    return RemoteFileArtifactValue.create(
-        digest(bytes), bytes.length, 1, "action-id", materializationExecPath);
+    return RemoteFileArtifactValue.create(digest(bytes), bytes.length, 1, materializationExecPath);
   }
 
   private static TreeArtifactValue createTreeMetadata(

src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java

@@ -208,8 +208,7 @@ private RemoteFileArtifactValue createRemoteMetadata(
             .getHashFunction()
             .hashBytes(bytes)
             .asBytes();
-    return RemoteFileArtifactValue.create(
-        digest, bytes.length, 1, "action-id", materializationExecPath);
+    return RemoteFileArtifactValue.create(digest, bytes.length, 1, materializationExecPath);
   }
 
   private RemoteFileArtifactValue createRemoteMetadata(Artifact artifact, String content) {

src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java

@@ -100,7 +100,6 @@ protected Artifact createRemoteArtifact(
             hashCode.asBytes(),
             contentsBytes.length,
             /* locationIndex= */ 1,
-            "action-id",
             materializationExecPath);
     metadata.put(a, f);
     if (cas != null) {
@@ -136,7 +135,7 @@ protected Pair<SpecialArtifact, ImmutableList<TreeFileArtifact>> createRemoteTre
           TreeFileArtifact.createTreeOutput(parent, PathFragment.create(entry.getKey()));
       RemoteFileArtifactValue childValue =
           RemoteFileArtifactValue.create(
-              hashCode.asBytes(), contentsBytes.length, /* locationIndex= */ 1, "action-id");
+              hashCode.asBytes(), contentsBytes.length, /* locationIndex= */ 1);
       treeBuilder.putChild(child, childValue);
       metadata.put(child, childValue);
       cas.put(hashCode, contentsBytes);

src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java

@@ -442,7 +442,7 @@ private Artifact createRemoteArtifact(
     byte[] b = contents.getBytes(StandardCharsets.UTF_8);
     HashCode h = HashCode.fromString(DIGEST_UTIL.compute(b).getHash());
     FileArtifactValue f =
-        RemoteFileArtifactValue.create(h.asBytes(), b.length, /* locationIndex= */ 1, "action-id");
+        RemoteFileArtifactValue.create(h.asBytes(), b.length, /* locationIndex= */ 1);
     inputs.putWithNoDepOwner(a, f);
     return a;
   }
@@ -513,8 +513,6 @@ public ListenableFuture<ImmutableSet<Digest>> findMissingDigests(
 
   private static class AllMissingDigestsFinder implements MissingDigestsFinder {
 
-    public static final AllMissingDigestsFinder INSTANCE = new AllMissingDigestsFinder();
-
     @Override
     public ListenableFuture<ImmutableSet<Digest>> findMissingDigests(
         RemoteActionExecutionContext context, Iterable<Digest> digests) {

src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java

@@ -326,7 +326,7 @@ protected void injectRemoteFile(FileSystem actionFs, PathFragment path, String c
     byte[] contentBytes = content.getBytes(StandardCharsets.UTF_8);
     HashCode hashCode = HASH_FUNCTION.getHashFunction().hashBytes(contentBytes);
     ((RemoteActionFileSystem) actionFs)
-        .injectRemoteFile(path, hashCode.asBytes(), contentBytes.length, "action-id");
+        .injectRemoteFile(path, hashCode.asBytes(), contentBytes.length);
   }
 
   @Override
@@ -342,7 +342,7 @@ private Artifact createRemoteArtifact(
     byte[] b = contents.getBytes(StandardCharsets.UTF_8);
     HashCode h = HASH_FUNCTION.getHashFunction().hashBytes(b);
     RemoteFileArtifactValue f =
-        RemoteFileArtifactValue.create(h.asBytes(), b.length, /* locationIndex= */ 1, "action-id");
+        RemoteFileArtifactValue.create(h.asBytes(), b.length, /* locationIndex= */ 1);
     inputs.putWithNoDepOwner(a, f);
     return a;
   }
@@ -364,8 +364,7 @@ private TreeArtifactValue createRemoteTreeArtifactValue(
       byte[] b = entry.getValue().getBytes(StandardCharsets.UTF_8);
       HashCode h = HASH_FUNCTION.getHashFunction().hashBytes(b);
       RemoteFileArtifactValue childMeta =
-          RemoteFileArtifactValue.create(
-              h.asBytes(), b.length, /* locationIndex= */ 0, "action-id");
+          RemoteFileArtifactValue.create(h.asBytes(), b.length, /* locationIndex= */ 0);
       builder.putChild(child, childMeta);
     }
     return builder.build();