examples: add new example to show updating metadata in interceptors #5788
Conversation
|
|
richzw
changed the title
|
@dfawley Could you please review my PR at your available time? |
@arvindbr8 Thank you for your review, I have moved the updating metadata in the interceptor into another folder named metadata_interceptor. Could you please review it again? |
| "google.golang.org/grpc/metadata" | ||
| "google.golang.org/grpc/status" | ||
|
|
||
| pb "google.golang.org/grpc/examples/features/proto/echo" |
There was a problem hiding this comment.
Do we need this change?
Although we don't do it at all places, we try (as much as possible) to group the proto imports separately.
There was a problem hiding this comment.
Sorry for the typo, it was caused by the Goland. I will fix it.
Documentation/grpc-metadata.md
Outdated
|
|
||
| ## Updating metadata in the interceptor - server side | ||
|
|
||
| Server side metadata updating in the interceptor examples are available [here](../examples/features/metadata_interceptor/server/main.go). |
There was a problem hiding this comment.
Please wrap these lines at 80 cols width. Thanks.
There was a problem hiding this comment.
Got it. I will fix it
There was a problem hiding this comment.
Could you please update this line to look like:
An example for updating metadata from a server interceptor is
available [here](../examples/features/metadata_interceptor/server/main.go).
Also, please ensure that this line is also wrapped to 80-cols width. Thanks.
Documentation/grpc-metadata.md
Outdated
|
|
||
| ```go | ||
| func SomeInterceptor(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) { | ||
| // get the metadata from context |
There was a problem hiding this comment.
Nit: Maybe replace with:
// Get the incoming metadata from the RPC context, and add a new
// key-value pair to it.
md, ok := metadata.FromIncomingContext(ctx)
md.Append("key1", "value1")
// Create a context with the new metadata and pass it to handler.
ctx = metadata.NewIncomingContext(ctx, md)
return handler(ctx, req)There was a problem hiding this comment.
Got it, sorry for my bad statement in the comment
Documentation/grpc-metadata.md
Outdated
| // get the metadata from context | ||
| md, ok := metadata.FromIncomingContext(ss.Context()) | ||
| // update metadata | ||
| md.Append("key1", "value1") | ||
| // set the metadata to context | ||
| ctx := metadata.NewIncomingContext(ss.Context(), md) | ||
|
|
||
| return handler(srv, &wrappedStream{ss, ctx}) |
There was a problem hiding this comment.
Similar comments here as the one above.
There was a problem hiding this comment.
Got it. I will fix it
| defer cancel() | ||
| resp, err := client.UnaryEcho(ctx, &pb.EchoRequest{Message: message}) | ||
| if err != nil { | ||
| log.Fatalf("client.UnaryEcho(_) = _, %v: ", err) |
There was a problem hiding this comment.
I know we do this in many (or all) examples, but I think we can print a simpler/nicer error message. Something like:
log.Fatalf("UnaryEcho: %v", err)There was a problem hiding this comment.
Got it. I will fix it
| break | ||
| } | ||
| if err != nil { | ||
| log.Fatalf("failed to receive response due to error: %v", err) |
There was a problem hiding this comment.
log.Fatalf("Receiving echo response: %v", err)There was a problem hiding this comment.
Got it. I will fix it
|
|
||
| func main() { | ||
| flag.Parse() | ||
| // Set up a connection to the server. |
There was a problem hiding this comment.
Nit: remove this comment as it is saying the obvious.
There was a problem hiding this comment.
Got it. I will fix it
| // Set up a connection to the server. | ||
| conn, err := grpc.Dial(*addr, grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
| if err != nil { | ||
| log.Fatalf("did not connect: %v", err) |
There was a problem hiding this comment.
log.Fatalf("grpc.Dial(%q): %v", *addr, err)There was a problem hiding this comment.
Got it. I will fix it
| ec := pb.NewEchoClient(conn) | ||
|
|
||
| callUnaryEcho(ec, message) | ||
| time.Sleep(1 * time.Second) |
There was a problem hiding this comment.
Why do we need this sleep?
There was a problem hiding this comment.
I will remove this sleep.
|
|
||
| var addr = flag.String("addr", "localhost:50051", "the address to connect to") | ||
|
|
||
| const message = "hello world" |
There was a problem hiding this comment.
Nit: Get rid of this const and move it inline to the calls to callUnaryEcho and callBidiStreamingEcho.
There was a problem hiding this comment.
Got it. Thank you for your view
@easwars Thank you very much for your detailed review. I have fixed the codes, comments and document. Could you review it again in your available time? I am looking forward for your server codes review. Thank you again. |
|
Argh, whoops. Something reminded me when I was looking at the flaky test: we should add this to https://github.com/grpc/grpc-go/blob/master/examples/examples_test.sh#L51 so that it is covered in our regression testing. |
|
@dfawley , thank you very much for your comment. The |
|
cc @temawi as FYI |
add updating metadata in the unary/stream interceptor to examples
RELEASE NOTES: