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]
> <javascript:>> 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] <javascript:>.
>> To post to this group, send email to [email protected]
>> <javascript:>.
>> 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/34768e0e-3aee-4d0b-9ea1-7a7a73a62c93%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
