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/CAPK2-4edGon4x6G1t3mC5D41aNibWKtE%2BHzVyPdpGeTyG%2ByoDw%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
smime.p7s
Description: S/MIME Cryptographic Signature
