Skip to content

Commit 83041b1

Browse files
coeuvrecoeuvre
authored
Refactor combined cache. (bazelbuild#16110)
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 bazelbuild#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. Closes bazelbuild#16039. PiperOrigin-RevId: 465577383 Change-Id: I99effab1cdcba0890671ea64c4660ea31b059ce7
1 parent e745468 commit 83041b1

File tree

9 files changed

+232
-256
lines changed

9 files changed

+232
-256
lines changed

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
import com.google.devtools.build.lib.events.Event;
3131
import com.google.devtools.build.lib.events.ExtendedEventHandler;
3232
import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
33-
import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext.Step;
33+
import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext.CachePolicy;
3434
import com.google.devtools.build.lib.remote.util.DigestUtil;
3535
import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
3636
import com.google.devtools.build.lib.vfs.Path;
@@ -274,8 +274,9 @@ private Single<PathConverter> upload(Set<Path> files) {
274274

275275
RequestMetadata metadata =
276276
TracingMetadataUtils.buildMetadata(buildRequestId, commandId, "bes-upload", null);
277-
RemoteActionExecutionContext context = RemoteActionExecutionContext.create(metadata);
278-
context.setStep(Step.UPLOAD_BES_FILES);
277+
RemoteActionExecutionContext context =
278+
RemoteActionExecutionContext.create(metadata)
279+
.withWriteCachePolicy(CachePolicy.REMOTE_CACHE_ONLY);
279280

280281
return Single.using(
281282
remoteCache::retain,

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

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -44,12 +44,11 @@ public static RemoteCacheClient createDiskAndRemoteClient(
4444
PathFragment diskCachePath,
4545
boolean remoteVerifyDownloads,
4646
DigestUtil digestUtil,
47-
RemoteCacheClient remoteCacheClient,
48-
RemoteOptions options)
47+
RemoteCacheClient remoteCacheClient)
4948
throws IOException {
5049
DiskCacheClient diskCacheClient =
5150
createDiskCache(workingDirectory, diskCachePath, remoteVerifyDownloads, digestUtil);
52-
return new DiskAndRemoteCacheClient(diskCacheClient, remoteCacheClient, options);
51+
return new DiskAndRemoteCacheClient(diskCacheClient, remoteCacheClient);
5352
}
5453

5554
public static RemoteCacheClient create(
@@ -147,12 +146,7 @@ private static RemoteCacheClient createDiskAndHttpCache(
147146

148147
RemoteCacheClient httpCache = createHttp(options, cred, digestUtil);
149148
return createDiskAndRemoteClient(
150-
workingDirectory,
151-
diskCachePath,
152-
options.remoteVerifyDownloads,
153-
digestUtil,
154-
httpCache,
155-
options);
149+
workingDirectory, diskCachePath, options.remoteVerifyDownloads, digestUtil, httpCache);
156150
}
157151

158152
public static boolean isDiskCache(RemoteOptions options) {

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

Lines changed: 97 additions & 59 deletions
Large diffs are not rendered by default.

‎src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -567,8 +567,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
567567
remoteOptions.diskCache,
568568
remoteOptions.remoteVerifyDownloads,
569569
digestUtil,
570-
cacheClient,
571-
remoteOptions);
570+
cacheClient);
572571
} catch (IOException e) {
573572
handleInitFailure(env, e, Code.CACHE_INIT_FAILURE);
574573
return;
@@ -626,8 +625,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
626625
remoteOptions.diskCache,
627626
remoteOptions.remoteVerifyDownloads,
628627
digestUtil,
629-
cacheClient,
630-
remoteOptions);
628+
cacheClient);
631629
} catch (IOException e) {
632630
handleInitFailure(env, e, Code.CACHE_INIT_FAILURE);
633631
return;

‎src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,10 @@ RemoteExecutionService getRemoteExecutionService() {
8282
@Override
8383
public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
8484
throws InterruptedException, IOException, ExecException, ForbiddenActionInputException {
85-
boolean shouldAcceptCachedResult = remoteExecutionService.shouldAcceptCachedResult(spawn);
86-
boolean shouldUploadLocalResults = remoteExecutionService.shouldUploadLocalResults(spawn);
85+
boolean shouldAcceptCachedResult =
86+
remoteExecutionService.getReadCachePolicy(spawn).allowAnyCache();
87+
boolean shouldUploadLocalResults =
88+
remoteExecutionService.getWriteCachePolicy(spawn).allowAnyCache();
8789
if (!shouldAcceptCachedResult && !shouldUploadLocalResults) {
8890
return SpawnCache.NO_RESULT_NO_STORE;
8991
}

‎src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -183,10 +183,11 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
183183
"Spawn can't be executed remotely. This is a bug.");
184184

185185
Stopwatch totalTime = Stopwatch.createStarted();
186-
boolean uploadLocalResults = remoteExecutionService.shouldUploadLocalResults(spawn);
187-
boolean acceptCachedResult = remoteExecutionService.shouldAcceptCachedResult(spawn);
186+
boolean acceptCachedResult = remoteExecutionService.getReadCachePolicy(spawn).allowAnyCache();
187+
boolean uploadLocalResults = remoteExecutionService.getWriteCachePolicy(spawn).allowAnyCache();
188188

189189
RemoteAction action = remoteExecutionService.buildRemoteAction(spawn, context);
190+
190191
SpawnMetrics.Builder spawnMetrics =
191192
SpawnMetrics.Builder.forRemoteExec()
192193
.setInputBytes(action.getInputBytes())

‎src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java‎

Lines changed: 74 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -13,48 +13,89 @@
1313
// limitations under the License.
1414
package com.google.devtools.build.lib.remote.common;
1515

16-
import build.bazel.remote.execution.v2.ExecuteResponse;
1716
import build.bazel.remote.execution.v2.RequestMetadata;
1817
import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
1918
import com.google.devtools.build.lib.actions.Spawn;
2019
import javax.annotation.Nullable;
2120

2221
/** A context that provide remote execution related information for executing an action remotely. */
2322
public class RemoteActionExecutionContext {
24-
/** The current step of the context. */
25-
public enum Step {
26-
INIT,
27-
CHECK_ACTION_CACHE,
28-
UPLOAD_INPUTS,
29-
EXECUTE_REMOTELY,
30-
UPLOAD_OUTPUTS,
31-
DOWNLOAD_OUTPUTS,
32-
UPLOAD_BES_FILES,
23+
/** Determines whether to read/write remote cache, disk cache or both. */
24+
public enum CachePolicy {
25+
NO_CACHE,
26+
REMOTE_CACHE_ONLY,
27+
DISK_CACHE_ONLY,
28+
ANY_CACHE;
29+
30+
public static CachePolicy create(boolean allowRemoteCache, boolean allowDiskCache) {
31+
if (allowRemoteCache && allowDiskCache) {
32+
return ANY_CACHE;
33+
} else if (allowRemoteCache) {
34+
return REMOTE_CACHE_ONLY;
35+
} else if (allowDiskCache) {
36+
return DISK_CACHE_ONLY;
37+
} else {
38+
return NO_CACHE;
39+
}
40+
}
41+
42+
public boolean allowAnyCache() {
43+
return this != NO_CACHE;
44+
}
45+
46+
public boolean allowRemoteCache() {
47+
return this == REMOTE_CACHE_ONLY || this == ANY_CACHE;
48+
}
49+
50+
public boolean allowDiskCache() {
51+
return this == DISK_CACHE_ONLY || this == ANY_CACHE;
52+
}
53+
54+
public CachePolicy addRemoteCache() {
55+
if (this == DISK_CACHE_ONLY || this == ANY_CACHE) {
56+
return ANY_CACHE;
57+
}
58+
59+
return REMOTE_CACHE_ONLY;
60+
}
3361
}
3462

3563
@Nullable private final Spawn spawn;
3664
private final RequestMetadata requestMetadata;
3765
private final NetworkTime networkTime;
38-
39-
@Nullable private ExecuteResponse executeResponse;
40-
private Step step;
66+
private final CachePolicy writeCachePolicy;
67+
private final CachePolicy readCachePolicy;
4168

4269
private RemoteActionExecutionContext(
4370
@Nullable Spawn spawn, RequestMetadata requestMetadata, NetworkTime networkTime) {
4471
this.spawn = spawn;
4572
this.requestMetadata = requestMetadata;
4673
this.networkTime = networkTime;
47-
this.step = Step.INIT;
74+
this.writeCachePolicy = CachePolicy.ANY_CACHE;
75+
this.readCachePolicy = CachePolicy.ANY_CACHE;
4876
}
4977

50-
/** Returns current {@link Step} of the context. */
51-
public Step getStep() {
52-
return step;
78+
private RemoteActionExecutionContext(
79+
@Nullable Spawn spawn,
80+
RequestMetadata requestMetadata,
81+
NetworkTime networkTime,
82+
CachePolicy writeCachePolicy,
83+
CachePolicy readCachePolicy) {
84+
this.spawn = spawn;
85+
this.requestMetadata = requestMetadata;
86+
this.networkTime = networkTime;
87+
this.writeCachePolicy = writeCachePolicy;
88+
this.readCachePolicy = readCachePolicy;
89+
}
90+
91+
public RemoteActionExecutionContext withWriteCachePolicy(CachePolicy writeCachePolicy) {
92+
return new RemoteActionExecutionContext(
93+
spawn, requestMetadata, networkTime, writeCachePolicy, readCachePolicy);
5394
}
5495

55-
/** Sets current {@link Step} of the context. */
56-
public void setStep(Step step) {
57-
this.step = step;
96+
public RemoteActionExecutionContext withReadCachePolicy(CachePolicy readCachePolicy) {
97+
return new RemoteActionExecutionContext(
98+
spawn, requestMetadata, networkTime, writeCachePolicy, readCachePolicy);
5899
}
59100

60101
/** Returns the {@link Spawn} of the action being executed or {@code null}. */
@@ -86,13 +127,12 @@ public ActionExecutionMetadata getSpawnOwner() {
86127
return spawn.getResourceOwner();
87128
}
88129

89-
public void setExecuteResponse(@Nullable ExecuteResponse executeResponse) {
90-
this.executeResponse = executeResponse;
130+
public CachePolicy getWriteCachePolicy() {
131+
return writeCachePolicy;
91132
}
92133

93-
@Nullable
94-
public ExecuteResponse getExecuteResponse() {
95-
return executeResponse;
134+
public CachePolicy getReadCachePolicy() {
135+
return readCachePolicy;
96136
}
97137

98138
/** Creates a {@link RemoteActionExecutionContext} with given {@link RequestMetadata}. */
@@ -108,4 +148,13 @@ public static RemoteActionExecutionContext create(
108148
@Nullable Spawn spawn, RequestMetadata metadata) {
109149
return new RemoteActionExecutionContext(spawn, metadata, new NetworkTime());
110150
}
151+
152+
public static RemoteActionExecutionContext create(
153+
@Nullable Spawn spawn,
154+
RequestMetadata requestMetadata,
155+
CachePolicy writeCachePolicy,
156+
CachePolicy readCachePolicy) {
157+
return new RemoteActionExecutionContext(
158+
spawn, requestMetadata, new NetworkTime(), writeCachePolicy, readCachePolicy);
159+
}
111160
}

0 commit comments

Comments
 (0)