Ok, updated the proposal to provide a full example without builders. On Monday, March 13, 2017 at 12:53:15 PM UTC-7, Michael Lumish wrote: > > Can you show in the proposal what the trivial implementation of all of the > interception methods would look like without the builders? > > On Mon, Mar 13, 2017 at 12:30 PM dduke via grpc.io < > [email protected] <javascript:>> wrote: > >> 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] <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/504cc473-66c5-4f1b-92f1-34f3414ccf15%40googlegroups.com >> >> <https://groups.google.com/d/msgid/grpc-io/504cc473-66c5-4f1b-92f1-34f3414ccf15%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/6f35b821-63ff-4936-8a08-f1290cf52f5d%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
