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/a7aa7063-1bb4-40a3-859c-7cfddd4f4838%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
