Bojan,
Thanks for the feedback. I think making wider use of the RPC type enums is
a good idea. I don't want to add any more complexity to this PR if I can
avoid it but I'd support a follow-up PR.
I'm open to simplifying the API syntax if the same functionality is
supported (all the functionality supported by the Java client interceptor
API). Ditching the interceptor object and making the `interceptCall`
function the 'interceptor' would be a simplification without removing
functionality. A simple intercepted call would then look like this:
const interceptor = function(options) {
return new InterceptingCall(options, {
sendMessage: function(message, next) {
next(_doSomething(message));
}
});
};
client.myCall(message, { interceptors: [interceptor] });
Thoughts?
On Thursday, March 9, 2017 at 7:06:04 PM UTC-8, [email protected] wrote:
>
> Hello,
>
> I do hope general feedback from community at large is welcome.
>
> +1 for adding const's and exports for method call types. This is something
> I feel has been needed and would be very useful. I think it might be better
> to export this as a separate package altogether, and grpc can depend on it;
> that way modules can require it and have access to grpc types without
> having to depend on the whole grpc node library, but that's somewhat a
> minor point and outside of the scope of this discussion.
>
> I do not have much in depth experience with gRPC in production, and just
> from my personal development (only with Node and a little with Go, no
> Java);.so maybe I am a little bit ignorant; but is there really a need for
> such a complicated and OOP-Y API for this? Javascript is function-first
> language with capability to pass around functions and objects alike simply
> and flexibly and most existing API's within the global Node.js ecosystem
> take this functional approach. We just need some way to expose an API to do
> pre, post hooks on events and provide means of composing middleware on
> client prototype functions? I really do not see why it would take 11 lines
> of code with 3 constructor invocations, and 2 build() calls to provide a
> mechanism to simply log a message being sent. What we really mean is just
> do this function before calling the client function.
>
> function(message, next) {
> logger.log(message);
> next(message);
> })
>
> Yet we need all this extra work just to accomplish it.
>
> Similarly for the retry example, given the choice of writing something
> like that code, or just using a single call to async.retry, the more
> readable code wins, even if you need some kind of custom logic.
>
> Couldn't we just provide some mechanism of passing an object specifying
> the hooks we want. ie something like:
>
> {
> onStart: function(... whatever) {
> // ...
> },
> onSend: function(message, next) {
>
> }, ...
> }
>
> etc...
>
> If needed we can document the interfaces the passed around objects may
> have to provide, if you need separate interfaces for listener, interceptor,
> etc... They can just be simple objects? To me having constructors and
> builders is adding additional noise that doesn't help readability and just
> makes things harder to reason about.
>
> I understand that this proposal is trying to expose something at a lower
> level so it doesn't have to account for different call contexts, etc, but
> given the ability that we can just pass around functions and arguments
> easily in javascript, I would think this should be doable.
> I haven't dug too much into it, so I might be wrong or maybe I am missing
> something. Even if conditional or contextual logic is needed, one could
> argue that that type of work falls precisely inside the implementation of
> the library providing the API (grpc).
>
> There are already existing examples of similar concepts of simpler
> functional hooks elsewhere in Node such as modules: kareem-hooks,
> hooks-fixed, grappling-hooks, and the hooks in mongoose.js for example.
>
> I wouldn't be surprised that if this was implemented eventually it would
> be abstracted into a higher level API to allow for a (IMO) saner usage. And
> maybe that's fine or intended.
>
> It's mentioned that a separate proposal for server interceptors using
> similar concepts and interfaces will be provided at a later date. Given the
> ubiquity of Express, and to smaller extend Restify and koa, all which
> provide API for composable middleware using just a simple `use` function
> and predetermined paradigms / idioms, I think similar API readability
> issues may be encountered. As a side project I've already created a simple
> module on top of existing grpc lib that provides similar API and
> functionality (https://github.com/malijs/mali). Not sure why exposing
> something complicated is needed. It is a personal side project that I can
> work on only as my life / time allows, but maybe when I get a chance I'll
> try and look into RFC process and try and document / propose something for
> server side. But again maybe the intention is to provide some kind of lower
> level typed interfaces, still leaving the ability to build on top to
> others, like what I've done.
>
> Anyway just my $0.02. Sorry I am not being negative on the proposal and
> since grpc is not something I use daily professionally at present I am
> probably somewhat ignorant and also not very invested. I am sure others in
> the community and this thread know better than me, but I am just trying to
> provide my objective opinion purely on the API based on my experience
> within the overall Node ecosystem.
>
> Regards,
>
> Bojan
>
> On Thursday, 9 March 2017 19:08:37 UTC-4, Michael Lumish wrote:
>>
>> It's probably fine to leave all of the options as they are now, but
>> options.credentials is a CallCredentials, not a ChannelCredentials. I think
>> that threw me off.
>>
>> On Thu, Mar 9, 2017 at 2:38 PM dduke via grpc.io <[email protected]>
>> wrote:
>>
>>> The proposal says "The interceptCall method allows developers to modify
>>>> the call options". What are the semantics of modifying these options,
>>>> particularly the ones that apply to an entire client, not a single method
>>>> (channelCredentials and host)? Is the MethodDescriptor object supposed to
>>>> be modifiable?
>>>
>>>
>>> The semantics of modifying the call options are whatever the semantics
>>> of modifying the options parameter to `getCall` in client.js are (
>>> https://github.com/grpc/grpc/blob/master/src/node/src/client.js#L332 )
>>>
>>> I could limit the options exposed to the options exposed in grpc-java's
>>> CallOptions class if that's preferrable:
>>>
>>> deadline
>>>
>>> authority (host)
>>>
>>> credentials
>>>
>>> customOptions
>>>
>>>
>>> The MethodDescriptor should not be modified. I've added a note for that.
>>>
>>>
>>> The three advanced examples use "StatusBuilder", but it doesn't seem to
>>>> be defined anywhere.
>>>
>>>
>>> I've added a full example of the StatusBuilder.
>>>
>>>
>>>> They also only appear to work for unary calls. This should at least be
>>>> mentioned.
>>>
>>>
>>> Each example lists the RPC types it supports:
>>>
>>> An example of a caching interceptor for *unary RPCs* which stores the
>>>> provided listener for later use (short-circuiting
>>>> the call if there is a cache hit):
>>>> An example retry interceptor for* unary RPCs* creates new calls when
>>>> the status shows a failure:
>>>> An example of providing fallbacks to failed requests for *unary or
>>>> client-streaming RPCs*:
>>>
>>>
>>> I'll add comments to the example code to repeat this.
>>>
>>>
>>>> In the Retry example, the call is retried for any non-OK status code.
>>>> There are some nuances that should be considered there, like idempotency
>>>> and the potential to increase traffic to overwhelmed servers that are
>>>> timing out. We already have a more complete proposal for handling retries
>>>> in the core at https://github.com/grpc/proposal/blob/master/A6.md, so
>>>> I think it may not be the best idea to encourage people to retry calls
>>>> with
>>>> a relatively simple implementation at a higher level.
>>>
>>>
>>> I can remove the retry example if it's not useful.
>>>
>>>
>>> Also, the Caching example appears to use "savedMessage" and
>>>> "storedMessage" interchangeably, and it does the same with
>>>> "savedListener" and "storedListener". And the proposal should say
>>>> "Implemented in: Javascript" at the top, instead of "Java, Go".
>>>
>>>
>>> Fixed.
>>>
>>>
>>>
>>> On Thursday, March 9, 2017 at 12:54:26 PM UTC-8, Michael Lumish wrote:
>>>
>>>> Overall, I think it looks better now, but I have a few nitpicks.
>>>>
>>>> The proposal says "The interceptCall method allows developers to modify
>>>> the call options". What are the semantics of modifying these options,
>>>> particularly the ones that apply to an entire client, not a single method
>>>> (channelCredentials and host)? Is the MethodDescriptor object supposed to
>>>> be modifiable?
>>>>
>>>> The three advanced examples use "StatusBuilder", but it doesn't seem to
>>>> be defined anywhere. They also only appear to work for unary calls. This
>>>> should at least be mentioned.
>>>>
>>>> In the Retry example, the call is retried for any non-OK status code.
>>>> There are some nuances that should be considered there, like idempotency
>>>> and the potential to increase traffic to overwhelmed servers that are
>>>> timing out. We already have a more complete proposal for handling retries
>>>> in the core at https://github.com/grpc/proposal/blob/master/A6.md, so
>>>> I think it may not be the best idea to encourage people to retry calls
>>>> with
>>>> a relatively simple implementation at a higher level.
>>>>
>>>> Also, the Caching example appears to use "savedMessage" and
>>>> "storedMessage" interchangeably, and it does the same with "savedListener"
>>>> and "storedListener". And the proposal should say "Implemented in:
>>>> Javascript" at the top, instead of "Java, Go".
>>>>
>>>> On Thu, Mar 9, 2017 at 10:58 AM dduke via grpc.io <
>>>> [email protected]> wrote:
>>>>
>>> The proposal and the PR are now updated with the new API:
>>>>> https://github.com/grpc/proposal/pull/14
>>>>> https://github.com/grpc/grpc/pull/9842
>>>>>
>>>>>
>>>>> On Friday, February 24, 2017 at 4:16:20 PM UTC-8, [email protected]
>>>>> wrote:
>>>>>>
>>>>>> Looking at this more closely, I think a more flexible
>>>>>> outbound/inbound interceptor API will cover the use cases I mentioned
>>>>>> without a separate batch-handling API. I'll work on updating the
>>>>>> proposal.
>>>>>>
>>>>>> On Friday, February 24, 2017 at 9:25:13 AM UTC-8, [email protected]
>>>>>> wrote:
>>>>>>>
>>>>>>> I agree with hiding the grpc.Call API. I'm concerned about the
>>>>>>> usability of the inbound/outbound interceptor API for use cases like
>>>>>>> caching. When we need to change how operations are batched or we want
>>>>>>> to
>>>>>>> short circuit the remaining interceptor stack, a purely
>>>>>>> operation-specific
>>>>>>> API is awkward and difficult to reason about.
>>>>>>>
>>>>>>> Would you consider an abstraction over the grpc.Call API like this?
>>>>>>>
>>>>>>> {
>>>>>>> interceptCall: function(callConstructor, options) {
>>>>>>> var call = callConstructor(options);
>>>>>>> return (new
>>>>>>> InterceptingCallBuilder(call)).withInterceptBatch(function(batch, next,
>>>>>>> responder) {
>>>>>>> var cachedResponse = _getCachedResponse(batch);
>>>>>>> if (cachedResponse) {
>>>>>>> var response = (new
>>>>>>> ResponseBuilder).withMessage(cachedResponse).withStatus(grpc.status.OK).build();
>>>>>>> responder(null, response);
>>>>>>> } else {
>>>>>>> next(batch);
>>>>>>> }
>>>>>>> }).build();
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> `batch` in this example would wrap the internal client_batch with
>>>>>>> some accessor methods: `hasMetadata`, `getMetadata`, etc. The returned
>>>>>>> call's `interceptBatch` would have access to (and the ability to
>>>>>>> mutate)
>>>>>>> the full batch and could short circuit the remaining interceptor stack
>>>>>>> without exposing the internal API.
>>>>>>>
>>>>>>>
>>>>>>> On Thursday, February 23, 2017 at 1:20:38 PM UTC-8, Michael Lumish
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> I do not like the idea of directly exposing the internal grpc.Call
>>>>>>>> API. It is internal for a reason, and it is not currently guaranteed
>>>>>>>> to be
>>>>>>>> stable. I would strongly prefer that the "outbound interceptor" and
>>>>>>>> "inbound interceptor" APIs be the fundamental building blocks of this
>>>>>>>> interceptor API.
>>>>>>>>
>>>>>>>> On Thursday, February 23, 2017 at 1:01:45 PM UTC-8, Michael Lumish
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> A proposal for a client interceptor API for the Node.js library
>>>>>>>>> has been created here: https://github.com/grpc/proposal/pull/14.
>>>>>>>>>
>>>>>>>>> Please keep discussion about it on this thread.
>>>>>>>>>
>>>>>>>> --
>>>>> 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 post to this group, send email to [email protected].
>>>>
>>>>
>>>>> Visit this group at https://groups.google.com/group/grpc-io.
>>>>> To view this discussion on the web visit
>>>>> https://groups.google.com/d/msgid/grpc-io/878bc96d-f6a7-4504-b106-6654b70de7d4%40googlegroups.com
>>>>>
>>>>> <https://groups.google.com/d/msgid/grpc-io/878bc96d-f6a7-4504-b106-6654b70de7d4%40googlegroups.com?utm_medium=email&utm_source=footer>
>>>>> .
>>>>> For more options, visit https://groups.google.com/d/optout.
>>>>>
>>>> --
>>> 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 post to this group, send email to [email protected].
>>> Visit this group at https://groups.google.com/group/grpc-io.
>>> To view this discussion on the web visit
>>> https://groups.google.com/d/msgid/grpc-io/0b852ae7-d5da-4302-9596-3d71884bc631%40googlegroups.com
>>>
>>> <https://groups.google.com/d/msgid/grpc-io/0b852ae7-d5da-4302-9596-3d71884bc631%40googlegroups.com?utm_medium=email&utm_source=footer>
>>> .
>>> For more options, visit https://groups.google.com/d/optout.
>>>
>>
--
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 post to this group, send email to [email protected].
Visit this group at https://groups.google.com/group/grpc-io.
To view this discussion on the web visit
https://groups.google.com/d/msgid/grpc-io/344a9c27-5732-4806-bf41-c8ed471c7a1b%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
