core: install the binary logging client interceptor #3937
Conversation
zpencer
changed the title
zpencer
force-pushed
the
binarylog-5-clientinterceptor-2-masterdiff
branch
from
cc9eae0 to
1c23481
Compare
| @@ -0,0 +1,19 @@ | |||
| syntax = "proto3"; | |||
There was a problem hiding this comment.
Why was this file added? It seems unused.
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
Still a lot of reviewing left, but sending what I have now.
| .setIdempotent(idempotent) | ||
| .setSafe(safe) | ||
| .setSampledToLocalTracing(sampledToLocalTracing); | ||
| .setSampledToLocalTracing(sampledToLocalTracing) |
There was a problem hiding this comment.
Could you split this bug fix into a separate PR? We may want to backport it, for instance.
There was a problem hiding this comment.
Posted #3941, but I'll leave this part alone so the test can work.
| return new SimpleForwardingClientCall<ReqT, RespT>(next.newCall(method, callOptions)) { | ||
| @Override | ||
| public void start(Listener<RespT> responseListener, Metadata headers) { | ||
| spyListener = responseListener = |
There was a problem hiding this comment.
Why this change? I'm happy to kill a spy, but I don't see what it has to do with the rest of the PR.
There was a problem hiding this comment.
Restructuring of ForwardingClientCallListener and PartialForwardingClientCallListener broke the spy, probably because the method we want is now an inherited method.
This works but is less obvious than the non spy():
spy(new SimpleForwardingClientCallListener(responseListener) {
@Override
public void onHeaders(Metadata headers) {
super.onHeaders(headers);
}
});
There was a problem hiding this comment.
It is broken by PartialForwardingClientCallListener being package-private. Making it public fixes it. But it's also fixed if I bump mockito to 2.13.0.
If we use AdditionalAnswers.delegatesTo instead of spy (which is what we should do anywhere we use spy, really), then it starts working:
spyListener = responseListener =
mock(ClientCall.Listener.class,
delegatesTo(new SimpleForwardingClientCallListener(responseListener) {}));Although then it can be simplified to:
spyListener = responseListener =
mock(ClientCall.Listener.class, delegatesTo(responseListener));
zpencer
force-pushed
the
binarylog-5-clientinterceptor-2-masterdiff
branch
from
5b2137b to
5b02a65
Compare
|
Rebased against master, resolved conflicts. @ejona86 PTAL |
| * so the interceptor must be reusable across server calls. At runtime, the request and response | ||
| * types passed into the interceptor is always {@link java.io.InputStream}. | ||
| * so the interceptor must be reusable across calls. At runtime, the request and response | ||
| * marshallers are always {@code Marshaller<Inputstream>}. |
| private static final class IdentityMarshaller implements Marshaller<InputStream> { | ||
| @Override | ||
| public InputStream stream(InputStream value) { | ||
| return value; |
- TODO for retry - methods should be public, for later provider interface - fix capitalization typo
2 participants
Some changes required to make this work:
MethodDescriptor.toBuilderso that it copies all fieldsBinaryLogProvider.providerreturnnullrather than the NullBinaryLogProvider. This is because on client side, the pipeline of interceptors is hard coded when the ManagedChannelImpl is created. It is desirable to avoid inserting a binary logging interceptor shim if we know there is no binary log.