Skip to content

L50: gRPC Objective-C Interceptor#140

Merged
muxi merged 5 commits intogrpc:masterfrom
muxi:l50-objc-interceptor
Jun 3, 2019
Merged

L50: gRPC Objective-C Interceptor#140
muxi merged 5 commits intogrpc:masterfrom
muxi:l50-objc-interceptor

Conversation

@muxi
Copy link
Contributor

@muxi muxi commented Apr 19, 2019

@muxi muxi requested a review from srini100 April 19, 2019 18:19
Copy link
Contributor

@srini100 srini100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few small nits since we already reviewed this offline.


gRPC interceptor has been a popular utility for users making HTTP requests.
gRPC Objective-C library has received a number of requests to add support of
this feature to gRPC. The old gRPC Objective-C library API was not flexiable enough to support interceptor API. With the [new API](https://github.com/grpc/proposal/blob/master/L38-objc-api-upgrade.md), now it is possible to add in this support.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/flexiable/flexible

* Collect metrics
* Obtain/Refresh access tokens
* Create test stub
* ...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caching should be a popular one, especially when you already have an example on it.

* Allows interceptors to specify their own dispatch queue
* Scope interceptors separately without direct access to each other's states
* Allows state sharing between interceptors when initiated by the interceptor author

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the above lines, clarify if "access" means read or write. If write, you could say something like "Allows modification to...."


### Related Proposals:
The interceptor API is built based on the proposals [L38: Proposal to A New gRPC Objective-C API](https://github.com/grpc/proposal/blob/master/L38-objc-api-upgrade.md) and [L49: gRPC Objective-C Flow Control](https://github.com/grpc/proposal/blob/c28ba74bbacc8c1fb5d269f48d89b06c51d21f44/L49-objc-flow-control.md).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flow control proposal is still marked as in review. It should be changed to approved.

### Interceptor Dispatch Queues
Since each interceptor is stateful, multiple requests or responses issued to an interceptor should be processed in order. That requires the dispatch queues of an interceptor to be serial queue. This requirement currently seems reasonable for us, because multiple calls can each have their own dispatch queue so that the same class of interceptors for different calls can still run concurrently. On the other hand, if serialized process is required across all interceptors of the same class, they will be able to use the same dispatch queue provided by their interceptor factory on initialization.

In case we have to support concurrent queue in the future, the following alternation may be used to achieve the goal transparently to the users and the interceptor developers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternation => alternative

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I didn't really mean "alternative" too :) But I rephrased it. Thanks Sanjay.


@end
```
After all the alternations in the interceptors, GRPCCall2Internal object uses the serializer to transform the data before passing it to core.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this was meant to be "alternatives" but probably you meant "alterations" as in updates? In that case "updates" itself might be a better word.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea that's correct :)

Copy link
Contributor

@srini100 srini100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@muxi muxi merged commit 13ec20c into grpc:master Jun 3, 2019
@muxi muxi deleted the l50-objc-interceptor branch June 3, 2019 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants