I've updated the gRPC PR and the proposal to promote the `interceptCall` function to represent the full interceptor, and also to clarify that use of the builders is optional.
On Monday, March 13, 2017 at 8:11:29 AM UTC-7, [email protected] wrote: > > > > On Monday, 13 March 2017 01:19:21 UTC-3, [email protected] wrote: >> >> 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] }); >> >> >> > Hello, > > Are you saying that this code above would work already in the current > proposal? With just setting properties within creation of new > InterceptingCall, and without having to use the InterceptorBuilder and > CallerBuilder? Sorry, that's not how I understood it but I may have missed > something. To me this code above is a lot more readable than having to use > the builders. If the above code would work with the existing proposal I'm > +1 :) > > My initial feedback was based on initial impressions when looking at the > proposal and reading through the examples given. I did not find the code as > readable and as intuitive as I think it could be, most of it being due to > the heavy builder approach. > > I understand fluent interfaces (pattern of construction via with* / > chained methods and builders as suggested) may make more sense for strongly > typed languages; IMO in Javascript they only make sense when dealing with > for DSL-y things where they help readability (like DB query builders for > example) or where they may actually have execution effects (ie stream > processing, and action chaining). So in this case personally I felt the > whole OOP and builder approach was perhaps a bit overkill and not very > intuitive. > > Having said that I did look into the Java documentation (I have 0 > experience with gRPC in Java) and it seems that this builder approach is > used extensively.It's also present in grpc-go within DialOption type. This > proposal PR seems very similar to the Java API. I suppose there could be an > argument made to keep consistency in the API between the platforms. If this > is a goal, I am fine with that, and more platform-appropriate abstractions > can be made on top if needed. I think there may already be some examples of > this within Go grpc eco system. > > Personally I do find the approach of keeping things simple and just > setting object properties more intuitive and in line with most existing > Javascript API's. I suppose in some ways it comes down to on project goals. > Do we want to keep API consistency between platforms, or tune the API to > follow more platform-specific approaches that may add to readability and > ease of entry (but go against consistency)? > > Anyway this is just my opinion. I am certainly not an authority on grpc > and if other more knowledgeable people have provided feedback and agreed to > the proposal that is cool with me. I don't want to hold anything back or > enter indecision deadlock and prevent this from moving through the proposal > process if everyone else is good with it. > > Thanks for reading, > > Bojan > > >> >> >> 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/504cc473-66c5-4f1b-92f1-34f3414ccf15%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
