Addressed review comments

SouthStar · Jul 20, 2016 · 9cb9445 · 9cb9445
1 parent c68640f
commit 9cb9445
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 18 deletions.
diff --git a/doc/command_line_tool.md b/doc/command_line_tool.md
@@ -15,6 +15,7 @@ The command line tool can do the following things:
 - Send unary rpc.
 - Attach metadata and display received metadata.
 - Handle common authentication to server.
+- Infer request/response types from server reflection result.
 - Find the request/response types from a given proto file.
 - Read proto request in text form.
 - Read request in wire form (for protobuf messages, this means serialized binary form).
@@ -24,7 +25,6 @@ The command line tool can do the following things:
 The command line tool should support the following things:
 
 - List server services and methods through server reflection.
-- Infer request/response types from server reflection result.
 - Fine-grained auth control (such as, use this oauth token to talk to the server).
 - Send streaming rpc.
 
@@ -46,24 +46,35 @@ https://github.com/grpc/grpc/blob/master/test/cpp/util/grpc_cli.cc
 Send a rpc to a helloworld server at `localhost:50051`:
 
 ```
-$ bins/opt/grpc_cli call localhost:50051 SayHello examples/protos/helloworld.proto \
-    "name: 'world'"  --enable_ssl=false
+$ bins/opt/grpc_cli call localhost:50051 SayHello "name: 'world'" \
+    --enable_ssl=false
 ```
 
 On success, the tool will print out
 
 ```
 Rpc succeeded with OK status
-Response: 
+Response:
  message: "Hello world"
 ```
 
 The `localhost:50051` part indicates the server you are connecting to. `SayHello` is (part of) the
-gRPC method string. Then there is the path to the proto file containing the service definition,
-if it is not under current directory, you can use `--proto_path` to specify a new search root.
-`"name: 'world'"` is the text format of the request proto message. 
-We are not using ssl here by `--enable_ssl=false`. For information on more
-flags, look at the comments of `grpc_cli.cc`.
+gRPC method string. Then `"name: 'world'"` is the text format of the request proto message. We are
+not using ssl here by `--enable_ssl=false`. For information on more flags, look at the comments of `grpc_cli.cc`.
+
+### Use local proto files
+
+If the server does not have the server reflection service, you will need to provide local proto
+files containing the service definition. The tool will try to find request/response types from
+them.
+
+```
+$ bins/opt/grpc_cli call localhost:50051 SayHello "name: 'world'" \
+    --protofiles=examples/protos/helloworld.proto --enable_ssl=false
+```
+
+If the proto files is not under current directory, you can use `--proto_path` to specify a new
+search root.
 
 ### Send non-proto rpc
 

diff --git a/test/cpp/util/grpc_cli.cc b/test/cpp/util/grpc_cli.cc
@@ -35,14 +35,14 @@
   A command line tool to talk to a grpc server.
   Example of talking to grpc interop server:
   grpc_cli call localhost:50051 UnaryCall "response_size:10" \
-      --proto_file=src/proto/grpc/testing/test.proto --enable_ssl=false
+      --protofiles=src/proto/grpc/testing/test.proto --enable_ssl=false
 
   Options:
-    1. --proto_file, use this flag to provide a proto file if the server does
+    1. --protofiles, use this flag to provide a proto file if the server does
        does not have the reflection service.
     2. --proto_path, if your proto file is not under current working directory,
        use this flag to provide a search root. It should work similar to the
-       counterpart in protoc. This option is valid only when proto_file is
+       counterpart in protoc. This option is valid only when protofiles is
        provided.
     3. --metadata specifies metadata to be sent to the server, such as:
        --metadata="MyHeaderKey1:Value1:MyHeaderKey2:Value2"
@@ -90,7 +90,8 @@ DEFINE_string(output_binary_file, "",
 DEFINE_string(metadata, "",
               "Metadata to send to server, in the form of key1:val1:key2:val2");
 DEFINE_string(proto_path, ".", "Path to look for the proto file.");
-DEFINE_string(proto_file, "", "Name of the proto file.");
+// TODO(zyc): support a list of input proto files
+DEFINE_string(protofiles, "", "Name of the proto file.");
 
 void ParseMetadataFlag(
     std::multimap<grpc::string, grpc::string>* client_metadata) {
@@ -173,9 +174,9 @@ int main(int argc, char** argv) {
   }
 
   if (!request_text.empty()) {
-    if (!FLAGS_proto_file.empty()) {
+    if (!FLAGS_protofiles.empty()) {
       parser.reset(new grpc::testing::ProtoFileParser(
-          FLAGS_proto_path, FLAGS_proto_file, method_name));
+          FLAGS_proto_path, FLAGS_protofiles, method_name));
     } else {
       parser.reset(new grpc::testing::ProtoFileParser(channel, method_name));
     }

diff --git a/test/cpp/util/proto_file_parser.cc b/test/cpp/util/proto_file_parser.cc
@@ -112,7 +112,7 @@ ProtoFileParser::ProtoFileParser(std::shared_ptr<grpc::Channel> channel,
     LogError(
         "Failed to get services from the server, "
         "it may not have the reflection service.\n"
-        "Please try to use the --proto_file option to provide a proto file.");
+        "Please try to use the --protofiles option to provide a proto file.");
   }
   if (has_error_) {
     return;

diff --git a/test/cpp/util/proto_reflection_descriptor_database.cc b/test/cpp/util/proto_reflection_descriptor_database.cc
@@ -54,8 +54,8 @@ ProtoReflectionDescriptorDatabase::ProtoReflectionDescriptorDatabase(
     : stub_(ServerReflection::NewStub(channel)) {}
 
 ProtoReflectionDescriptorDatabase::~ProtoReflectionDescriptorDatabase() {
-  if (!stream_) {
-    GetStream()->WritesDone();
+  if (stream_) {
+    stream_->WritesDone();
     Status status = stream_->Finish();
     if (!status.ok()) {
       gpr_log(GPR_ERROR,