-
Notifications
You must be signed in to change notification settings - Fork 773
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve extensibility of gRPC invocation #669
Conversation
I've greatly reduced the new API surface. Now there is just one new public method. Some code has moved to a shared source folder. If we ever decided to not share it then its simple to move it back again. Ready for review @jtattermusch |
Can you give a list of publicly visible changes? I went through the changes and noticed that besides, the new |
Overall looks lood, added a few questions. I'd also like a detailed review from @JunTaoLuo |
Are the changes from #677 required for this PR? If not, I'd prefer merging the two separately. |
cd2b288
to
c7e45b6
Compare
c7e45b6
to
4cb62b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor nits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Public changes:
Internal changes:
GrpcServiceOptions
(mutable version). Lays the groundwork for per-method customization of settings.. Update: Now internal and shared source. We can decide whether to make public in the future.