> > 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] > <javascript:>> 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] <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/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. For more options, visit https://groups.google.com/d/optout.
