Thanks for the detailed response Eric. Appreciate it. Find my responses inline.
On Mon, Jul 20, 2020 at 11:02 AM Eric Anderson <[email protected]> wrote: > 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. > - My bad. You are right. Implementing ClientInterceptor would > suffice in my proposal. > > 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. > - Gotcha. > > 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. > thanks. Can. you throw some light on what more power you are insinuating. > > 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. > - My impl is just a draft and I missed to add onReady(). I will > checkout 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. > - got it, thanks. > > 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. > - Yeah, I see that. not all interceptors are interested in response info. > > 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. > - Yeah, even I had the same question when I was reading and trying to understand grpc, as to why start() has to take in the headers rather than initialization. > > 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. > - Yes, I realized that after posting it to the group. > > 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. > - Yeah, I haven't thought much about failing/cancelling a request from an interceptor. Will see if how I can abstract that out as well. Thanks for your feedback Eric once again. -- Regards, -Sivabalan -- 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/CABeKz3mVU%2Bk5dn2wU%2BjzBvo_2LtU4fE9oFBvymOi9h7%3DtsZKjQ%40mail.gmail.com.
