Skip to content

Commit 35642f4

Browse files
coeuvrecopybara-github
coeuvre
authored and
copybara-github
committed
Delay the creation of remoteServerInstanceName.
We need `Channel#authority()` to determine `remoteServerInstanceName` when uploading artifacts for BEP. However, we don't need to create it inside `RemoteModule#beforeCommand`, otherwise it has to wait for gRPC channel creation and server capabilities verification. This CL changes to lazily get the `remoteServerInstanceName` right before it's needed. Working towards #18607. PiperOrigin-RevId: 562740380 Change-Id: I05a55e5d9a024f6d6d506da22691b76696b82d6c
1 parent 25d739b commit 35642f4

File tree

10 files changed

+84
-38
lines changed

10 files changed

+84
-38
lines changed

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

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

16+
import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
1617
import static com.google.devtools.build.lib.remote.util.DigestUtil.isOldStyleDigestFunction;
1718
import static com.google.devtools.build.lib.remote.util.RxFutures.toCompletable;
1819
import static com.google.devtools.build.lib.remote.util.RxFutures.toListenableFuture;
@@ -24,6 +25,7 @@
2425
import build.bazel.remote.execution.v2.RequestMetadata;
2526
import com.google.common.base.Ascii;
2627
import com.google.common.base.Preconditions;
28+
import com.google.common.base.Strings;
2729
import com.google.common.collect.ImmutableSet;
2830
import com.google.common.collect.Maps;
2931
import com.google.common.collect.Sets;
@@ -76,7 +78,8 @@ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted
7678
private final RemoteCache remoteCache;
7779
private final String buildRequestId;
7880
private final String commandId;
79-
private final String remoteServerInstanceName;
81+
private final String remoteInstanceName;
82+
private final String remoteBytestreamUriPrefix;
8083

8184
private final AtomicBoolean shutdown = new AtomicBoolean();
8285
private final Scheduler scheduler;
@@ -93,7 +96,8 @@ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted
9396
ExtendedEventHandler reporter,
9497
boolean verboseFailures,
9598
RemoteCache remoteCache,
96-
String remoteServerInstanceName,
99+
String remoteInstanceName,
100+
String remoteBytestreamUriPrefix,
97101
String buildRequestId,
98102
String commandId,
99103
XattrProvider xattrProvider,
@@ -104,7 +108,8 @@ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted
104108
this.remoteCache = remoteCache;
105109
this.buildRequestId = buildRequestId;
106110
this.commandId = commandId;
107-
this.remoteServerInstanceName = remoteServerInstanceName;
111+
this.remoteInstanceName = remoteInstanceName;
112+
this.remoteBytestreamUriPrefix = remoteBytestreamUriPrefix;
108113
this.scheduler = Schedulers.from(executor);
109114
this.xattrProvider = xattrProvider;
110115
this.remoteBuildEventUploadMode = remoteBuildEventUploadMode;
@@ -367,6 +372,21 @@ private Single<List<PathMetadata>> uploadLocalFiles(
367372
.collect(Collectors.toList());
368373
}
369374

375+
private Single<String> getRemoteServerInstanceName(RemoteCache remoteCache) {
376+
if (!Strings.isNullOrEmpty(remoteBytestreamUriPrefix)) {
377+
return Single.just(remoteBytestreamUriPrefix);
378+
}
379+
380+
return toSingle(remoteCache.cacheProtocol::getAuthority, directExecutor())
381+
.map(
382+
a -> {
383+
if (!Strings.isNullOrEmpty(remoteInstanceName)) {
384+
return a + "/" + remoteInstanceName;
385+
}
386+
return a;
387+
});
388+
}
389+
370390
private Single<PathConverter> doUpload(Map<Path, LocalFile> files) {
371391
if (files.isEmpty()) {
372392
return Single.just(PathConverter.NO_CONVERSION);
@@ -403,10 +423,15 @@ private Single<PathConverter> doUpload(Map<Path, LocalFile> files) {
403423
.collect(Collectors.toList())
404424
.flatMap(paths -> queryRemoteCache(remoteCache, context, paths))
405425
.flatMap(paths -> uploadLocalFiles(remoteCache, context, paths))
406-
.map(
426+
.flatMap(
407427
paths ->
408-
new PathConverterImpl(
409-
remoteServerInstanceName, paths, remoteBuildEventUploadMode)),
428+
getRemoteServerInstanceName(remoteCache)
429+
.map(
430+
remoteServerInstanceName ->
431+
new PathConverterImpl(
432+
remoteServerInstanceName,
433+
paths,
434+
remoteBuildEventUploadMode))),
410435
RemoteCache::release);
411436
}
412437

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,8 @@ class ByteStreamBuildEventArtifactUploaderFactory implements BuildEventArtifactU
3030
private final ExtendedEventHandler reporter;
3131
private final boolean verboseFailures;
3232
private final RemoteCache remoteCache;
33-
private final String remoteServerInstanceName;
33+
private final String remoteInstanceName;
34+
private final String remoteBytestreamUriPrefix;
3435
private final String buildRequestId;
3536
private final String commandId;
3637
private final RemoteBuildEventUploadMode remoteBuildEventUploadMode;
@@ -42,15 +43,17 @@ class ByteStreamBuildEventArtifactUploaderFactory implements BuildEventArtifactU
4243
ExtendedEventHandler reporter,
4344
boolean verboseFailures,
4445
RemoteCache remoteCache,
45-
String remoteServerInstanceName,
46+
String remoteInstanceName,
47+
String remoteBytestreamUriPrefix,
4648
String buildRequestId,
4749
String commandId,
4850
RemoteBuildEventUploadMode remoteBuildEventUploadMode) {
4951
this.executor = executor;
5052
this.reporter = reporter;
5153
this.verboseFailures = verboseFailures;
5254
this.remoteCache = remoteCache;
53-
this.remoteServerInstanceName = remoteServerInstanceName;
55+
this.remoteInstanceName = remoteInstanceName;
56+
this.remoteBytestreamUriPrefix = remoteBytestreamUriPrefix;
5457
this.buildRequestId = buildRequestId;
5558
this.commandId = commandId;
5659
this.remoteBuildEventUploadMode = remoteBuildEventUploadMode;
@@ -65,7 +68,8 @@ public BuildEventArtifactUploader create(CommandEnvironment env) {
6568
reporter,
6669
verboseFailures,
6770
remoteCache.retain(),
68-
remoteServerInstanceName,
71+
remoteInstanceName,
72+
remoteBytestreamUriPrefix,
6973
buildRequestId,
7074
commandId,
7175
env.getXattrProvider(),

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,11 @@ public CacheCapabilities getCacheCapabilities() {
266266
return channel.getServerCapabilities().getCacheCapabilities();
267267
}
268268

269+
@Override
270+
public ListenableFuture<String> getAuthority() {
271+
return channel.withChannelFuture(ch -> Futures.immediateFuture(ch.authority()));
272+
}
273+
269274
@Override
270275
public ListenableFuture<CachedActionResult> downloadActionResult(
271276
RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) {
@@ -521,4 +526,8 @@ ListenableFuture<Void> uploadChunker(
521526
Retrier getRetrier() {
522527
return this.retrier;
523528
}
529+
530+
public ReferenceCountedChannel getChannel() {
531+
return channel;
532+
}
524533
}

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

Lines changed: 2 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,6 @@
100100
import com.google.devtools.common.options.OptionsBase;
101101
import com.google.devtools.common.options.OptionsParsingResult;
102102
import io.grpc.CallCredentials;
103-
import io.grpc.Channel;
104103
import io.grpc.ClientInterceptor;
105104
import io.grpc.ManagedChannel;
106105
import io.netty.handler.codec.DecoderException;
@@ -544,22 +543,6 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
544543
}
545544
}
546545

547-
String remoteBytestreamUriPrefix = remoteOptions.remoteBytestreamUriPrefix;
548-
if (Strings.isNullOrEmpty(remoteBytestreamUriPrefix)) {
549-
try {
550-
remoteBytestreamUriPrefix = cacheChannel.withChannelBlocking(Channel::authority);
551-
} catch (Exception e) {
552-
if (e instanceof InterruptedException) {
553-
Thread.currentThread().interrupt();
554-
}
555-
handleInitFailure(env, e, Code.CACHE_INIT_FAILURE);
556-
return;
557-
}
558-
if (!Strings.isNullOrEmpty(remoteOptions.remoteInstanceName)) {
559-
remoteBytestreamUriPrefix += "/" + remoteOptions.remoteInstanceName;
560-
}
561-
}
562-
563546
RemoteCacheClient cacheClient =
564547
new GrpcCacheClient(
565548
cacheChannel.retain(), callCredentialsProvider, remoteOptions, retrier, digestUtil);
@@ -652,7 +635,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
652635
env.getReporter(),
653636
verboseFailures,
654637
actionContextProvider.getRemoteCache(),
655-
remoteBytestreamUriPrefix,
638+
remoteOptions.remoteInstanceName,
639+
remoteOptions.remoteBytestreamUriPrefix,
656640
buildRequestId,
657641
invocationId,
658642
remoteOptions.remoteBuildEventUploadMode));

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@
3434
public interface RemoteCacheClient extends MissingDigestsFinder {
3535
CacheCapabilities getCacheCapabilities();
3636

37+
ListenableFuture<String> getAuthority();
38+
3739
/**
3840
* A key in the remote action cache. The type wraps around a {@link Digest} of an {@link Action}.
3941
* Action keys are special in that they aren't content-addressable but refer to action results.

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,11 @@ public CacheCapabilities getCacheCapabilities() {
206206
return remoteCache.getCacheCapabilities();
207207
}
208208

209+
@Override
210+
public ListenableFuture<String> getAuthority() {
211+
return remoteCache.getAuthority();
212+
}
213+
209214
@Override
210215
public ListenableFuture<CachedActionResult> downloadActionResult(
211216
RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) {

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

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
// limitations under the License.
1414
package com.google.devtools.build.lib.remote.disk;
1515

16+
import static com.google.common.util.concurrent.Futures.immediateFuture;
1617
import static com.google.devtools.build.lib.remote.util.DigestUtil.isOldStyleDigestFunction;
1718

1819
import build.bazel.remote.execution.v2.ActionCacheUpdateCapabilities;
@@ -97,7 +98,7 @@ private ListenableFuture<Void> download(Digest digest, OutputStream out, boolean
9798
} else {
9899
try (InputStream in = p.getInputStream()) {
99100
ByteStreams.copy(in, out);
100-
return Futures.immediateFuture(null);
101+
return immediateFuture(null);
101102
} catch (IOException e) {
102103
return Futures.immediateFailedFuture(e);
103104
}
@@ -117,7 +118,7 @@ public ListenableFuture<Void> downloadBlob(
117118
Utils.verifyBlobContents(digest, digestOut.digest());
118119
}
119120
out.flush();
120-
return Futures.immediateFuture(null);
121+
return immediateFuture(null);
121122
} catch (IOException e) {
122123
return Futures.immediateFailedFuture(e);
123124
}
@@ -169,6 +170,11 @@ public CacheCapabilities getCacheCapabilities() {
169170
.build();
170171
}
171172

173+
@Override
174+
public ListenableFuture<String> getAuthority() {
175+
return immediateFuture("");
176+
}
177+
172178
@Override
173179
public ListenableFuture<CachedActionResult> downloadActionResult(
174180
RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) {
@@ -177,16 +183,16 @@ public ListenableFuture<CachedActionResult> downloadActionResult(
177183
actionKey, (digest, out) -> download(digest, out, /* isActionCache= */ true)),
178184
actionResult -> {
179185
if (actionResult == null) {
180-
return Futures.immediateFuture(null);
186+
return immediateFuture(null);
181187
}
182188

183189
try {
184190
checkActionResult(actionResult);
185191
} catch (CacheNotFoundException e) {
186-
return Futures.immediateFuture(null);
192+
return immediateFuture(null);
187193
}
188194

189-
return Futures.immediateFuture(CachedActionResult.disk(actionResult));
195+
return immediateFuture(CachedActionResult.disk(actionResult));
190196
},
191197
MoreExecutors.directExecutor());
192198
}
@@ -196,7 +202,7 @@ public ListenableFuture<Void> uploadActionResult(
196202
RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) {
197203
try (InputStream data = actionResult.toByteString().newInput()) {
198204
saveFile(actionKey.getDigest().getHash(), data, /* actionResult= */ true);
199-
return Futures.immediateFuture(null);
205+
return immediateFuture(null);
200206
} catch (IOException e) {
201207
return Futures.immediateFailedFuture(e);
202208
}
@@ -213,7 +219,7 @@ public ListenableFuture<Void> uploadFile(
213219
} catch (IOException e) {
214220
return Futures.immediateFailedFuture(e);
215221
}
216-
return Futures.immediateFuture(null);
222+
return immediateFuture(null);
217223
}
218224

219225
@Override
@@ -224,15 +230,15 @@ public ListenableFuture<Void> uploadBlob(
224230
} catch (IOException e) {
225231
return Futures.immediateFailedFuture(e);
226232
}
227-
return Futures.immediateFuture(null);
233+
return immediateFuture(null);
228234
}
229235

230236
@Override
231237
public ListenableFuture<ImmutableSet<Digest>> findMissingDigests(
232238
RemoteActionExecutionContext context, Iterable<Digest> digests) {
233239
// Both upload and download check if the file exists before doing I/O. So we don't
234240
// have to do it here.
235-
return Futures.immediateFuture(ImmutableSet.copyOf(digests));
241+
return immediateFuture(ImmutableSet.copyOf(digests));
236242
}
237243

238244
protected Path toPathNoSplit(String key) {

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -609,6 +609,11 @@ public CacheCapabilities getCacheCapabilities() {
609609
.build();
610610
}
611611

612+
@Override
613+
public ListenableFuture<String> getAuthority() {
614+
return Futures.immediateFuture("");
615+
}
616+
612617
@Override
613618
public ListenableFuture<CachedActionResult> downloadActionResult(
614619
RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) {

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -586,7 +586,8 @@ private ByteStreamBuildEventArtifactUploader newArtifactUploader(RemoteCache rem
586586
reporter,
587587
/* verboseFailures= */ true,
588588
remoteCache,
589-
/* remoteServerInstanceName= */ "localhost/instance",
589+
/* remoteInstanceName= */ "",
590+
/* remoteBytestreamUriPrefix= */ "localhost/instance",
590591
/* buildRequestId= */ "none",
591592
/* commandId= */ "none",
592593
SyscallCache.NO_CACHE,

‎src/test/java/com/google/devtools/build/lib/remote/util/InMemoryCacheClient.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,11 @@ public CacheCapabilities getCacheCapabilities() {
111111
return CacheCapabilities.getDefaultInstance();
112112
}
113113

114+
@Override
115+
public ListenableFuture<String> getAuthority() {
116+
return Futures.immediateFuture("");
117+
}
118+
114119
@Override
115120
public ListenableFuture<CachedActionResult> downloadActionResult(
116121
RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) {

0 commit comments

Comments
 (0)