That's the same API as proposed, except with the interceptCall function
promoted to represent the interceptor. Maybe the examples are confusing
because they use the builders? This is the same example but using the API
in the proposal:
const interceptor = {
interceptCall: function(options) {
return new InterceptingCall(options, {
sendMessage: function(message, next) {
next(_doSomething(message));
}
});
}
};
client.myCall(message, { interceptors: [interceptor] });
On Saturday, March 11, 2017 at 4:25:40 PM UTC-8, [email protected] wrote:
>
> Hello,
>
> +1 If that can be made to work to satisfy all the functionality that
> you're looking for, personally I think that API is MUCH better and more
> readable :)
>
> Regards,
> Bojan
>
> On Friday, 10 March 2017 18:22:15 UTC-4, [email protected] wrote:
>>
>> 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/6684876c-0f9b-4ac1-b103-a304a830d239%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
