Implement missing authority API for HandlerRegistry #2704
Conversation
|
Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure. |
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I'm not done with this (style, tests, etc) but am looking for feedback on the concept. Am I going in the right direction here? |
| import io.grpc.internal.GrpcUtil; | ||
| import io.grpc.internal.ServerTransportListener; | ||
| import io.grpc.internal.StatsTraceContext; | ||
| import io.grpc.internal.*; |
There was a problem hiding this comment.
We don't do * style imports.
| ServerMethodDefinition<?, ?> method = registry.lookupMethod(methodName); | ||
| if (method == null) { | ||
| method = fallbackRegistry.lookupMethod(methodName); | ||
| String authority = stream.getAttributes() |
There was a problem hiding this comment.
Attributes.newBuilder(Attributes)
There was a problem hiding this comment.
I don't understand this comment. Can you elaborate?
There was a problem hiding this comment.
Oops, that should be in NettyServerHandler.java line 197
| this, http2Stream, maxMessageSize, statsTraceCtx); | ||
| NettyServerStream stream = new NettyServerStream(ctx.channel(), state, attributes, | ||
| Attributes attributesWithAuthority = Attributes | ||
| .newBuilder() |
There was a problem hiding this comment.
This file is not particularly good about following the rule, but line continuation should be +4.
| NettyServerStream stream = new NettyServerStream(ctx.channel(), state, attributes, | ||
| Attributes attributesWithAuthority = Attributes | ||
| .newBuilder() | ||
| .setAll(attributes).set(ServerImpl.ATTR_AUTHORITY, headers.authority().toString()) |
There was a problem hiding this comment.
This is going to add extra allocation and AsciiString conversion in the serving path, where there was none before. Almost no users need this data. This would slow down all servers for the sake of proxies.
There was a problem hiding this comment.
Is there a cleaner way to get authority up to the callbackRegistry that doesn't involve passing authority along the server path?
|
|
4 participants
HandlerRegistry.lookupMethod(String methodName, String authority)is never never invoked with a non-null authority string. Per the discussion on the mailing list, I implemented this method.https://groups.google.com/forum/#!topic/grpc-io/1RPka9CKAvE