Skip to content

Commit

Permalink
BuildEventServiceGrpcClient accepts ClientInterceptor in constructor.
Browse files Browse the repository at this point in the history 
With this refactoring, the `BuildEventServiceGrpcClient` is not concerned with
interpreting command-line options and can operate purely on gRPC domain objects.
The `BazelBuildEventServiceModule` is responsible for preparing the domain objects
from the command-line options.

This refactoring was requested but skipped during PR #13812 because the
BazelBuildEventServiceModuleTest was not yet open-sourced. Now that the test is
open-sourced we can more easily separate (and test) these concerns.

PiperOrigin-RevId: 391347857
  • Loading branch information
@michaeledgar @copybara-github
michaeledgar authored and copybara-github committed Aug 17, 2021
1 parent cfc2d1d commit f141e14
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 34 deletions.
43 changes: 33 additions & 10 deletions ...in/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,18 @@
import com.google.devtools.build.lib.authandtls.GoogleAuthUtils;
import com.google.devtools.build.lib.buildeventservice.client.BuildEventServiceClient;
import com.google.devtools.build.lib.buildeventservice.client.BuildEventServiceGrpcClient;
import io.grpc.ClientInterceptor;
import io.grpc.ManagedChannel;
import io.grpc.Metadata;
import io.grpc.stub.MetadataUtils;
import java.io.IOException;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Objects;
import java.util.Set;
import javax.annotation.Nullable;

/**
* Bazel's BES module.
*/
/** Bazel's BES module. */
public class BazelBuildEventServiceModule
extends BuildEventServiceModule<BuildEventServiceOptions> {

Expand All @@ -47,6 +49,15 @@ abstract static class BackendConfig {
abstract ImmutableList<Map.Entry<String, String>> besHeaders();

abstract AuthAndTLSOptions authAndTLSOptions();

static BackendConfig create(
BuildEventServiceOptions besOptions, AuthAndTLSOptions authAndTLSOptions) {
return new AutoValue_BazelBuildEventServiceModule_BackendConfig(
besOptions.besBackend,
besOptions.besProxy,
ImmutableMap.copyOf(besOptions.besHeaders).entrySet().asList(),
authAndTLSOptions);
}
}

private BuildEventServiceClient client;
Expand All @@ -60,24 +71,36 @@ protected Class<BuildEventServiceOptions> optionsClass() {
@Override
protected BuildEventServiceClient getBesClient(
BuildEventServiceOptions besOptions, AuthAndTLSOptions authAndTLSOptions) throws IOException {
BackendConfig newConfig =
new AutoValue_BazelBuildEventServiceModule_BackendConfig(
besOptions.besBackend,
besOptions.besProxy,
ImmutableMap.copyOf(besOptions.besHeaders).entrySet().asList(),
authAndTLSOptions);
BackendConfig newConfig = BackendConfig.create(besOptions, authAndTLSOptions);
if (client == null || !Objects.equals(config, newConfig)) {
clearBesClient();
config = newConfig;
client =
new BuildEventServiceGrpcClient(
newGrpcChannel(config),
GoogleAuthUtils.newCallCredentials(config.authAndTLSOptions()),
config.besHeaders());
makeGrpcInterceptor(config));
}
return client;
}

private static ClientInterceptor makeGrpcInterceptor(BackendConfig config) {
if (config.besHeaders().isEmpty()) {
return null;
}
return MetadataUtils.newAttachHeadersInterceptor(makeGrpcMetadata(config));
}

@VisibleForTesting
static Metadata makeGrpcMetadata(BackendConfig config) {
Metadata extraHeaders = new Metadata();
for (Entry<String, String> header : config.besHeaders()) {
extraHeaders.put(
Metadata.Key.of(header.getKey(), Metadata.ASCII_STRING_MARSHALLER), header.getValue());
}
return extraHeaders;
}

// newGrpcChannel is only defined so it can be overridden in tests to not use a real network link.
@VisibleForTesting
protected ManagedChannel newGrpcChannel(BackendConfig config) throws IOException {
Expand Down
16 changes: 1 addition & 15 deletions ...a/com/google/devtools/build/lib/buildeventservice/client/BuildEventServiceGrpcClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,12 @@
import io.grpc.CallCredentials;
import io.grpc.ClientInterceptor;
import io.grpc.ManagedChannel;
import io.grpc.Metadata;
import io.grpc.Status;
import io.grpc.StatusException;
import io.grpc.StatusRuntimeException;
import io.grpc.stub.AbstractStub;
import io.grpc.stub.MetadataUtils;
import io.grpc.stub.StreamObserver;
import java.time.Duration;
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;

/** Implementation of BuildEventServiceClient that uploads data using gRPC. */
Expand All @@ -55,17 +51,7 @@ public class BuildEventServiceGrpcClient implements BuildEventServiceClient {
public BuildEventServiceGrpcClient(
ManagedChannel channel,
@Nullable CallCredentials callCredentials,
List<Map.Entry<String, String>> headers) {
ClientInterceptor interceptor = null;
if (!headers.isEmpty()) {
Metadata extraHeaders = new Metadata();
headers.forEach(
header ->
extraHeaders.put(
Metadata.Key.of(header.getKey(), Metadata.ASCII_STRING_MARSHALLER),
header.getValue()));
interceptor = MetadataUtils.newAttachHeadersInterceptor(extraHeaders);
}
ClientInterceptor interceptor) {
this.besAsync =
configureStub(PublishBuildEventGrpc.newStub(channel), callCredentials, interceptor);
this.besBlocking =
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/com/google/devtools/build/lib/buildeventservice/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ java_test(
"//src/test/java/com/google/devtools/build/lib/testutil",
"//third_party:guava",
"//third_party:junit4",
"//third_party:mockito",
"//third_party:truth",
"//third_party/grpc:grpc-jar",
"//third_party/protobuf:protobuf_java",
Expand All @@ -48,6 +47,7 @@ java_test(
deps = [
"//src/main/java/com/google/devtools/build/lib:runtime",
"//src/main/java/com/google/devtools/build/lib/actions:action_lookup_data",
"//src/main/java/com/google/devtools/build/lib/authandtls",
"//src/main/java/com/google/devtools/build/lib/bugreport",
"//src/main/java/com/google/devtools/build/lib/buildeventservice",
"//src/main/java/com/google/devtools/build/lib/buildeventservice:buildeventservice-options",
Expand Down
24 changes: 24 additions & 0 deletions ...ava/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,15 @@
import com.google.common.base.MoreObjects;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Sets;
import com.google.common.util.concurrent.Uninterruptibles;
import com.google.devtools.build.lib.actions.ActionLookupData;
import com.google.devtools.build.lib.analysis.util.AnalysisMock;
import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions;
import com.google.devtools.build.lib.bugreport.BugReport;
import com.google.devtools.build.lib.buildeventservice.BazelBuildEventServiceModule.BackendConfig;
import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader;
import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.Aborted;
Expand Down Expand Up @@ -66,6 +69,7 @@
import com.google.devtools.build.v1.StreamId;
import com.google.protobuf.Empty;
import io.grpc.ManagedChannel;
import io.grpc.Metadata;
import io.grpc.Server;
import io.grpc.Status;
import io.grpc.StatusRuntimeException;
Expand Down Expand Up @@ -588,6 +592,26 @@ public void testKeywords() throws Exception {
.containsExactly("user_keyword=keyword0", "user_keyword=keyword1");
}

@Test
public void testMakeGrpcMetadata() throws Exception {
runBuildWithOptions();
BuildEventServiceOptions besOptions = new BuildEventServiceOptions();
AuthAndTLSOptions authAndTLSOptions = new AuthAndTLSOptions();
besOptions.besBackend = "bes-backend";
besOptions.besProxy = "bes-proxy";
besOptions.besHeaders =
ImmutableMap.of("key1", "val1", "key2", "val2", "key3", "val3").entrySet().asList();
BackendConfig newConfig = BackendConfig.create(besOptions, authAndTLSOptions);

Metadata metadata = BazelBuildEventServiceModule.makeGrpcMetadata(newConfig);
assertThat(metadata.get(Metadata.Key.of("key1", Metadata.ASCII_STRING_MARSHALLER)))
.isEqualTo("val1");
assertThat(metadata.get(Metadata.Key.of("key2", Metadata.ASCII_STRING_MARSHALLER)))
.isEqualTo("val2");
assertThat(metadata.get(Metadata.Key.of("key3", Metadata.ASCII_STRING_MARSHALLER)))
.isEqualTo("val3");
}

/** Regression test for b/111653523. */
@Test
public void testCoverageFileIncluded() throws Exception {
Expand Down
16 changes: 8 additions & 8 deletions ...java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceGrpcClientTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,11 @@

import static com.google.common.truth.Truth.assertThat;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.Maps;
import com.google.devtools.build.lib.buildeventservice.client.BuildEventServiceGrpcClient;
import com.google.devtools.build.v1.PublishBuildEventGrpc;
import com.google.devtools.build.v1.PublishBuildToolEventStreamRequest;
import com.google.devtools.build.v1.PublishBuildToolEventStreamResponse;
import io.grpc.ClientInterceptor;
import io.grpc.ManagedChannel;
import io.grpc.Metadata;
import io.grpc.Server;
Expand All @@ -34,6 +33,7 @@
import io.grpc.StatusException;
import io.grpc.inprocess.InProcessChannelBuilder;
import io.grpc.inprocess.InProcessServerBuilder;
import io.grpc.stub.MetadataUtils;
import io.grpc.stub.StreamObserver;
import java.util.ArrayList;
import java.util.UUID;
Expand Down Expand Up @@ -116,11 +116,11 @@ public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(
return next.startCall(call, headers);
}
}))) {
Metadata extraHeaders = new Metadata();
extraHeaders.put(Metadata.Key.of("metadata-foo", Metadata.ASCII_STRING_MARSHALLER), "bar");
ClientInterceptor interceptor = MetadataUtils.newAttachHeadersInterceptor(extraHeaders);
BuildEventServiceGrpcClient grpcClient =
new BuildEventServiceGrpcClient(
server.getChannel(),
null,
ImmutableList.of(Maps.immutableEntry("metadata-foo", "bar")));
new BuildEventServiceGrpcClient(server.getChannel(), null, interceptor);
assertThat(grpcClient.openStream(ack -> {}).getStatus().get()).isEqualTo(Status.OK);
assertThat(seenHeaders).hasSize(1);
Metadata headers = seenHeaders.get(0);
Expand All @@ -133,7 +133,7 @@ public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(
public void immediateSuccess() throws Exception {
try (TestServer server = startTestServer(NOOP_SERVER.bindService())) {
assertThat(
new BuildEventServiceGrpcClient(server.getChannel(), null, ImmutableList.of())
new BuildEventServiceGrpcClient(server.getChannel(), null, null)
.openStream(ack -> {})
.getStatus()
.get())
Expand All @@ -154,7 +154,7 @@ public StreamObserver<PublishBuildToolEventStreamRequest> publishBuildToolEventS
}
}.bindService())) {
assertThat(
new BuildEventServiceGrpcClient(server.getChannel(), null, ImmutableList.of())
new BuildEventServiceGrpcClient(server.getChannel(), null, null)
.openStream(ack -> {})
.getStatus()
.get())
Expand Down

0 comments on commit f141e14

Please sign in to comment.