On Thu, Jul 16, 2020 at 9:32 AM Sivabalan <[email protected]> wrote:
> This is not a breaking change, my proposal is to introduce a simpler
> abstract class which devs can choose to extend rather than implementing
> ClientInterceptor.
>
ClientInterceptors are really just a small tweak to a Channel to allow them
to be plugged together by passing Channel next. Your
SimpleClientInterceptor should implement ClientInterceptor instead of
InterceptorChannel and then it'd be a drop-in option anywhere
ClientInterceptor is available today.
You shouldn't generally make interceptors implement ManagedChannel because
then it is hard to determine which channel should be shut down, and when it
is shut down which other channels it will shut down.
I come from java land and so the proposal is in java, but should be
> generalizable.
>
> public abstract class SimpleClientInterceptor<ReqT, RespT> {
>
> /**
> * Invoked when a message is sent out from client to server.
> * @param message message being sent out.
> */
> protected abstract void onRequestMessage(ReqT message);
>
This mainly supports "observing" client interceptors, that don't need to
make any changes to the RPC other than Metadata. That helps many, but lots
of interceptors need more power than this. It's still fine to have a
simpler version in certain scenarios.
Here is the impl draft
> <https://gist.github.com/nsivabalan/a60789c97e7d67bb743a64ea3396a163> on
> how we could achieve this. Basic idea is to add a ClientCallHandler/wrapper
> and call into the interceptor impl where ever required.
>
As I said above, SimpleClientInterceptor should implement ClientInterceptor
instead of InterceptorChannel. ClientCallHandler would be simpler/better if
it extended SimpleFowardingClientCall. Your InternalListener would break
flow control currently, as it does not forward onReady. It would be better
to extend SimpleForwardingClientCallListener.
As of today, an interceptor which just processes headers looks complicated
> as below.
>
> // process request headers. add/edit/remove headers.
> super.start(new
> SimpleForwardingClientCallListener<RespT>(responseListener) {
> @Override
> public void onHeaders(Metadata headers) {
> super.onHeaders(headers);
> }
> }, headers);
> ...
>
> @Override
> public String authority() {
> return next.authority();
> }
> }
>
>
authority() is not part of the interceptor API, so that would not be
present. Also, since it is not interested in response headers (only request
headers), it can use super.start(responseListener, headers); and avoid
the SimpleForwardingClientCallListener
usage.
That's actually an important point: many interceptors don't need response
information, so they don't need to wrap the Listener. That is a benefit as
less indirection occurs for events.
I do think it was a mistake to have Metadata passed to start() instead of
Channel.newCall()/ClientInterceptor.interceptCall(). That would have
simplified things for interceptors that only need the request Metadata. It
was less clear in the day when the decision was made, because of some
tradeoffs.
With the new proposal, an interceptor which accesses some headers would be
> like below.
>
> @Override
>
> protected void onRequestMessage(ReqT message) { }
>
I suggest you have default noop implementations for all the methods. That
way implementations don't need to override all methods.
Implementors don't need to worry about calling next entity in the chain as
> its taken care by the CallHandler/wrapper. More details can be checked out
> here <https://gist.github.com/nsivabalan/a60789c97e7d67bb743a64ea3396a163> on
> how to get this done.
>
> Just a reminder, that this is not a breaking change, just that a new
> abstract class will be introduced which devs can use if they want to,
> instead of the ClientInterceptor interface directly.
>
I think this is a good and great thing for you to have and share with
others. I wouldn't be as excited for it to be in io.grpc itself, though, as
many interceptors can't be implemented using it. That causes a burden
because users then need to convert their interceptor to a different API
when they need certain control, and it wouldn't generally be obvious to
them that the API they are using doesn't support whatever specific thing
they need at the time.
If it *just* had Metadata, especially request metadata, then its limits
would be more obvious. But users frequently ask how they fail a call, and
you can't fail a call with this type of API. I'm not sure users would
realize the API has that limitation. But with some documentation maybe that
could be addressed.
--
You received this message because you are subscribed to the Google Groups
"grpc.io" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/grpc-io/CA%2B4M1oPm9Q9SzPuVZj6g1UB%2Bc3LELxU4Q8mqkH1tEDWOVvcOew%40mail.gmail.com.
smime.p7s
Description: S/MIME Cryptographic Signature
