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/7797c4dc-91cd-4eab-93a3-a23f9301191d%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
