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]> 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]. > 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 > <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/CAPK2-4ci2vAyNuRok-AG5g9-KA_td02C370NZRVXEFJsyXP%3DXQ%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
smime.p7s
Description: S/MIME Cryptographic Signature
