Added calloption to retrieve peer information #1066
Conversation
rpc_util.go
Outdated
| @@ -141,6 +142,7 @@ type callInfo struct { | |||
| headerMD metadata.MD | |||
| trailerMD metadata.MD | |||
| traceInfo traceInfo // in trace.go | |||
| peer peer.Peer | |||
There was a problem hiding this comment.
Move this above traceInfo?
rpc_util.go
Outdated
| @@ -183,6 +185,13 @@ func Trailer(md *metadata.MD) CallOption { | |||
| }) | |||
| } | |||
|
|
|||
| // Peer returns a CallOption that retrieves peer information. | |||
There was a problem hiding this comment.
Add "for a unary RPC" at the end so it's consistent with Header and Trailer?
test/end2end_test.go
Outdated
| if _, err := tc.EmptyCall(context.Background(), &testpb.Empty{}, grpc.Peer(peer)); err != nil { | ||
| t.Fatalf("TestService/EmptyCall(_, _) = _, %v, want _, <nil>", err) | ||
| } | ||
| pa := extractPort(peer.Addr.String()) |
There was a problem hiding this comment.
Use net.SplitHostPort?
https://golang.org/pkg/net/#SplitHostPort
test/end2end_test.go
Outdated
| defer te.tearDown() | ||
| tc := testpb.NewTestServiceClient(te.clientConn()) | ||
| peer := new(peer.Peer) | ||
| if _, err := tc.EmptyCall(context.Background(), &testpb.Empty{}, grpc.Peer(peer)); err != nil { |
rpc_util.go
Outdated
| @@ -140,6 +141,7 @@ type callInfo struct { | |||
| failFast bool | |||
| headerMD metadata.MD | |||
| trailerMD metadata.MD | |||
| peer peer.Peer | |||
There was a problem hiding this comment.
Is it better to use a *peer.Peer here?
So that we don't need to copy peer in recvResponse.
There was a problem hiding this comment.
- In such a case the function Peer(peer *peer.Peer) would now need to take a double pointer argument: Peer(peer *peer.Peer).
- Providing a pointer to the original object doesn't have much use since there's no use case that requires the user to update the object.
- All these changes only enable one less copy of the object which seems like over-optimization.
There was a problem hiding this comment.
We can still make a copy and return the copy of Peer to the user, so we can avoid the double pointer.
I was thinking that we should avoid copying Peer inside gRPC.
Assume most users don't care about peer, so they won't use this new CallOption. In that case, we don't need to copy Peer and return it. But the copy in recvResponse will still happen.
There was a problem hiding this comment.
I see your point. Yes, it makes sense to remove this copy from the codepath of the users that don't care about this value.
No description provided.