Intercept capabilities and uploader requests and add custom grpc headers #10634
Conversation
AlessandroPatti
requested review from
buchgr and
ola-rozenfeld
as code owners
|
Thanks Ale for patching this bug ! |
|
Friendly ping! |
| @@ -87,6 +89,7 @@ | |||
|
|
|||
| @GuardedBy("lock") | |||
| private final Map<HashCode, ListenableFuture<Void>> uploadsInProgress = new HashMap<>(); | |||
| private final ClientInterceptor[] interceptors; | |||
There was a problem hiding this comment.
I'd prefer to use ImmutableList here instead of array. One problem with using arrays is that anyone with a reference to the array can modify the contents asynchronously. Technically, ensuring the safety of this code requires reviewing all the callers to check if they're doing anything sinister. Even if it's unlikely that anyone would do so intentionally, it wouldn't be obvious if anyone is doing it accidentally.
There was a problem hiding this comment.
Changed this one to an ImmutableList as suggested.
There was also a similar bit in RemoteServerCapability, for which I opted for making a copy of the array instead, as it makes it easier to pass it directly to the withInterceptors method, that only accepts an array.
| retrier); | ||
| retrier, | ||
| (execChannel != null ? | ||
| TracingMetadataUtils.newExecHeadersInterceptor(remoteOptions) : |
There was a problem hiding this comment.
I find it slightly odd that this has different options for caching and execution. If someone is asking Bazel to do one or the other, they can also provide different options, no?
(I realize that's a pre-existing condition, so feel free to ignore this comment.)
There was a problem hiding this comment.
Updated to run the checks twice if the endpoints differ.
| @@ -103,6 +105,10 @@ public static void afterEverything() { | |||
| assertThat(meta.getToolDetails().getToolName()).isEqualTo("bazel"); | |||
| assertThat(meta.getToolDetails().getToolVersion()) | |||
| .isEqualTo(BlazeVersionInfo.instance().getVersion()); | |||
| assertThat(headers.get(Metadata.Key.of("Key1", Metadata.ASCII_STRING_MARSHALLER))) | |||
There was a problem hiding this comment.
Having checks in the common case means that any future test case is forced to also test this by setting up custom remoteOptions. That seems like a bad direction, generally speaking.
There was a problem hiding this comment.
Right, I'll create a new test case for it.
AlessandroPatti
force-pushed
the
apatti/grpc-headers-fix
branch
from
4958e52
to
93986c1
Compare
Yes, just an oversight. |
AlessandroPatti
force-pushed
the
apatti/grpc-headers-fix
branch
from
93986c1
to
493c573
Compare
| @@ -108,14 +113,16 @@ public ByteStreamUploader( | |||
| ReferenceCountedChannel channel, | |||
| @Nullable CallCredentials callCredentials, | |||
| long callTimeoutSecs, | |||
| RemoteRetrier retrier) { | |||
| RemoteRetrier retrier, | |||
| ClientInterceptor... interceptors) { | |||
There was a problem hiding this comment.
Did you consider passing in a channel that already has the interceptors attached instead? It seems preferable to me to not add knowledge about interceptors to the ByteStreamUploader if not necessary.
AlessandroPatti
force-pushed
the
apatti/grpc-headers-fix
branch
from
9deee90
to
d153b31
Compare
Following #10015. Some requests do not use the custom headers.