From a3ef19c1f0a318f5c0d8985cf6b956707cdf8199 Mon Sep 17 00:00:00 2001 From: Alisha Nanda Date: Wed, 9 Sep 2020 18:32:27 -0700 Subject: [PATCH] Revert "Add timeout flag to gRPC cli" --- CMakeLists.txt | 3 - build_autogenerated.yaml | 6 - src/proto/grpc/testing/echo.proto | 9 +- src/proto/grpc/testing/simple_messages.proto | 8 +- test/cpp/util/BUILD | 2 - test/cpp/util/cli_call.cc | 19 +-- test/cpp/util/cli_call.h | 6 +- test/cpp/util/cli_flags.cc | 53 -------- test/cpp/util/cli_flags.h | 46 ------- test/cpp/util/grpc_tool.cc | 48 +++++-- test/cpp/util/grpc_tool.h | 5 +- test/cpp/util/grpc_tool_test.cc | 128 ++----------------- 12 files changed, 61 insertions(+), 272 deletions(-) delete mode 100644 test/cpp/util/cli_flags.cc delete mode 100644 test/cpp/util/cli_flags.h diff --git a/CMakeLists.txt b/CMakeLists.txt index c4eede491fd..43fc3b6536a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -10079,7 +10079,6 @@ add_executable(cli_call_test test/cpp/util/cli_call.cc test/cpp/util/cli_call_test.cc test/cpp/util/cli_credentials.cc - test/cpp/util/cli_flags.cc test/cpp/util/grpc_tool.cc test/cpp/util/proto_file_parser.cc test/cpp/util/proto_reflection_descriptor_database.cc @@ -11188,7 +11187,6 @@ add_executable(grpc_cli ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/reflection/v1alpha/reflection.grpc.pb.h test/cpp/util/cli_call.cc test/cpp/util/cli_credentials.cc - test/cpp/util/cli_flags.cc test/cpp/util/grpc_cli.cc test/cpp/util/grpc_tool.cc test/cpp/util/proto_file_parser.cc @@ -11592,7 +11590,6 @@ if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_MAC OR _gRPC_PLATFORM_POSIX) ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/simple_messages.grpc.pb.h test/cpp/util/cli_call.cc test/cpp/util/cli_credentials.cc - test/cpp/util/cli_flags.cc test/cpp/util/grpc_tool.cc test/cpp/util/grpc_tool_test.cc test/cpp/util/proto_file_parser.cc diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index a181e90142a..537c473c184 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -5416,7 +5416,6 @@ targets: headers: - test/cpp/util/cli_call.h - test/cpp/util/cli_credentials.h - - test/cpp/util/cli_flags.h - test/cpp/util/config_grpc_cli.h - test/cpp/util/grpc_tool.h - test/cpp/util/proto_file_parser.h @@ -5430,7 +5429,6 @@ targets: - test/cpp/util/cli_call.cc - test/cpp/util/cli_call_test.cc - test/cpp/util/cli_credentials.cc - - test/cpp/util/cli_flags.cc - test/cpp/util/grpc_tool.cc - test/cpp/util/proto_file_parser.cc - test/cpp/util/proto_reflection_descriptor_database.cc @@ -5871,7 +5869,6 @@ targets: headers: - test/cpp/util/cli_call.h - test/cpp/util/cli_credentials.h - - test/cpp/util/cli_flags.h - test/cpp/util/config_grpc_cli.h - test/cpp/util/grpc_tool.h - test/cpp/util/proto_file_parser.h @@ -5881,7 +5878,6 @@ targets: - src/proto/grpc/reflection/v1alpha/reflection.proto - test/cpp/util/cli_call.cc - test/cpp/util/cli_credentials.cc - - test/cpp/util/cli_flags.cc - test/cpp/util/grpc_cli.cc - test/cpp/util/grpc_tool.cc - test/cpp/util/proto_file_parser.cc @@ -5990,7 +5986,6 @@ targets: headers: - test/cpp/util/cli_call.h - test/cpp/util/cli_credentials.h - - test/cpp/util/cli_flags.h - test/cpp/util/config_grpc_cli.h - test/cpp/util/grpc_tool.h - test/cpp/util/proto_file_parser.h @@ -6002,7 +5997,6 @@ targets: - src/proto/grpc/testing/simple_messages.proto - test/cpp/util/cli_call.cc - test/cpp/util/cli_credentials.cc - - test/cpp/util/cli_flags.cc - test/cpp/util/grpc_tool.cc - test/cpp/util/grpc_tool_test.cc - test/cpp/util/proto_file_parser.cc diff --git a/src/proto/grpc/testing/echo.proto b/src/proto/grpc/testing/echo.proto index c4233035dbc..db583a1305b 100644 --- a/src/proto/grpc/testing/echo.proto +++ b/src/proto/grpc/testing/echo.proto @@ -15,17 +15,15 @@ syntax = "proto3"; -package grpc.testing; - import "src/proto/grpc/testing/echo_messages.proto"; import "src/proto/grpc/testing/simple_messages.proto"; +package grpc.testing; + service EchoTestService { rpc Echo(EchoRequest) returns (EchoResponse); rpc Echo1(EchoRequest) returns (EchoResponse); rpc Echo2(EchoRequest) returns (EchoResponse); - rpc CheckDeadlineUpperBound(SimpleRequest) returns (StringValue); - rpc CheckDeadlineSet(SimpleRequest) returns (StringValue); // A service which checks that the initial metadata sent over contains some // expected key value pair rpc CheckClientInitialMetadata(SimpleRequest) returns (SimpleResponse); @@ -66,4 +64,5 @@ service UnimplementedEchoService { } // A service without any rpc defined to test coverage. -service NoRpcService {} +service NoRpcService { +} diff --git a/src/proto/grpc/testing/simple_messages.proto b/src/proto/grpc/testing/simple_messages.proto index 3afe236b459..4b65d18909d 100644 --- a/src/proto/grpc/testing/simple_messages.proto +++ b/src/proto/grpc/testing/simple_messages.proto @@ -17,10 +17,8 @@ syntax = "proto3"; package grpc.testing; -message SimpleRequest {} - -message SimpleResponse {} +message SimpleRequest { +} -message StringValue { - string message = 1; +message SimpleResponse { } diff --git a/test/cpp/util/BUILD b/test/cpp/util/BUILD index 8f638d75cba..21bf8535d90 100644 --- a/test/cpp/util/BUILD +++ b/test/cpp/util/BUILD @@ -122,14 +122,12 @@ grpc_cc_library( srcs = [ "cli_call.cc", "cli_credentials.cc", - "cli_flags.cc", "proto_file_parser.cc", "service_describer.cc", ], hdrs = [ "cli_call.h", "cli_credentials.h", - "cli_flags.h", "config_grpc_cli.h", "proto_file_parser.h", "service_describer.h", diff --git a/test/cpp/util/cli_call.cc b/test/cpp/util/cli_call.cc index c57633ee4a3..049588f860f 100644 --- a/test/cpp/util/cli_call.cc +++ b/test/cpp/util/cli_call.cc @@ -18,16 +18,15 @@ #include "test/cpp/util/cli_call.h" +#include +#include + #include #include #include #include #include #include -#include - -#include -#include namespace grpc { namespace testing { @@ -62,18 +61,6 @@ CliCall::CliCall(const std::shared_ptr& channel, ctx_.AddMetadata(iter->first, iter->second); } } - - // Set deadline if timeout > 0 (default value -1 if no timeout specified) - if (FLAGS_timeout > 0) { - int64_t timeout_in_ns = ceil(FLAGS_timeout * 1e9); - - // Convert timeout (in microseconds) to a deadline - auto deadline = - gpr_time_add(gpr_now(GPR_CLOCK_MONOTONIC), - gpr_time_from_nanos(timeout_in_ns, GPR_TIMESPAN)); - ctx_.set_deadline(deadline); - } - call_ = stub_->PrepareCall(&ctx_, method, &cq_); call_->StartCall(tag(1)); void* got_tag; diff --git a/test/cpp/util/cli_call.h b/test/cpp/util/cli_call.h index a03c7495560..4afa25d194e 100644 --- a/test/cpp/util/cli_call.h +++ b/test/cpp/util/cli_call.h @@ -19,16 +19,14 @@ #ifndef GRPC_TEST_CPP_UTIL_CLI_CALL_H #define GRPC_TEST_CPP_UTIL_CLI_CALL_H +#include + #include #include #include #include #include -#include - -#include "test/cpp/util/cli_flags.h" - namespace grpc { class ClientContext; diff --git a/test/cpp/util/cli_flags.cc b/test/cpp/util/cli_flags.cc deleted file mode 100644 index fdf910d4dbb..00000000000 --- a/test/cpp/util/cli_flags.cc +++ /dev/null @@ -1,53 +0,0 @@ -/* - * - * Copyright 2016 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -#include "test/cpp/util/cli_flags.h" - -namespace grpc { -namespace testing { - -// Define all flags used in gRPC cli. -DEFINE_bool(l, false, "Use a long listing format"); -DEFINE_bool(remotedb, true, "Use server types to parse and format messages"); -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(protofiles, "", "Name of the proto file."); -DEFINE_bool(binary_input, false, "Input in binary format"); -DEFINE_bool(binary_output, false, "Output in binary format"); -DEFINE_string( - default_service_config, "", - "Default service config to use on the channel, if non-empty. Note that " - "this will be ignored if the name resolver returns a service config."); -DEFINE_bool(display_peer_address, false, - "Log the peer socket address of the connection that each RPC is " - "made on to stderr."); -DEFINE_bool(json_input, false, "Input in json format"); -DEFINE_bool(json_output, false, "Output in json format"); -DEFINE_string(infile, "", "Input file (default is stdin)"); -DEFINE_bool(batch, false, - "Input contains multiple requests. Please do not use this to send " - "more than a few RPCs. gRPC CLI has very different performance " - "characteristics compared with normal RPC calls which make it " - "unsuitable for loadtesting or significant production traffic."); -DEFINE_double(timeout, -1, - "Specify timeout in seconds, used to set the deadline for all " - "RPCs. The default value of -1 means no deadline has been set."); - -} // namespace testing -} // namespace grpc diff --git a/test/cpp/util/cli_flags.h b/test/cpp/util/cli_flags.h deleted file mode 100644 index c8e41c57fc3..00000000000 --- a/test/cpp/util/cli_flags.h +++ /dev/null @@ -1,46 +0,0 @@ -/* - * - * Copyright 2016 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -#ifndef GRPC_TEST_CPP_UTIL_CLI_FLAGS_H -#define GRPC_TEST_CPP_UTIL_CLI_FLAGS_H - -#include - -namespace grpc { -namespace testing { - -// Declare all flags used in gRPC cli. -DECLARE_bool(l); -DECLARE_bool(remotedb); -DECLARE_string(metadata); -DECLARE_string(proto_path); -DECLARE_string(protofiles); -DECLARE_bool(binary_input); -DECLARE_bool(binary_output); -DECLARE_string(default_service_config); -DECLARE_bool(display_peer_address); -DECLARE_bool(json_input); -DECLARE_bool(json_output); -DECLARE_string(infile); -DECLARE_bool(batch); -DECLARE_double(timeout); - -} // namespace testing -} // namespace grpc - -#endif // GRPC_TEST_CPP_UTIL_CLI_FLAGS_H diff --git a/test/cpp/util/grpc_tool.cc b/test/cpp/util/grpc_tool.cc index df473ad795a..27a9bdc3665 100644 --- a/test/cpp/util/grpc_tool.cc +++ b/test/cpp/util/grpc_tool.cc @@ -18,14 +18,6 @@ #include "test/cpp/util/grpc_tool.h" -#include -#include -#include -#include -#include -#include -#include - #include #include #include @@ -34,6 +26,15 @@ #include #include +#include +#include +#include +#include +#include +#include +#include +#include + #include "test/cpp/util/cli_call.h" #include "test/cpp/util/proto_file_parser.h" #include "test/cpp/util/proto_reflection_descriptor_database.h" @@ -48,6 +49,32 @@ namespace grpc { namespace testing { +DEFINE_bool(l, false, "Use a long listing format"); +DEFINE_bool(remotedb, true, "Use server types to parse and format messages"); +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(protofiles, "", "Name of the proto file."); +DEFINE_bool(binary_input, false, "Input in binary format"); +DEFINE_bool(binary_output, false, "Output in binary format"); +DEFINE_string( + default_service_config, "", + "Default service config to use on the channel, if non-empty. Note " + "that this will be ignored if the name resolver returns a service " + "config."); +DEFINE_bool( + display_peer_address, false, + "Log the peer socket address of the connection that each RPC is made " + "on to stderr."); +DEFINE_bool(json_input, false, "Input in json format"); +DEFINE_bool(json_output, false, "Output in json format"); +DEFINE_string(infile, "", "Input file (default is stdin)"); +DEFINE_bool(batch, false, + "Input contains multiple requests. Please do not use this to send " + "more than a few RPCs. gRPC CLI has very different performance " + "characteristics compared with normal RPC calls which make it " + "unsuitable for loadtesting or significant production traffic."); + namespace { class GrpcTool { @@ -463,10 +490,7 @@ bool GrpcTool::CallMethod(int argc, const char** argv, " --binary_input ; Input in binary format\n" " --binary_output ; Output in binary format\n" " --json_input ; Input in json format\n" - " --json_output ; Output in json format\n" - " --timeout ; Specify timeout (in seconds), used to " - "set the deadline for RPCs. The default value of -1 means no " - "deadline has been set.\n" + + " --json_output ; Output in json format\n" + cred.GetCredentialUsage()); std::stringstream output_ss; diff --git a/test/cpp/util/grpc_tool.h b/test/cpp/util/grpc_tool.h index b5441c5eea7..e998807cc61 100644 --- a/test/cpp/util/grpc_tool.h +++ b/test/cpp/util/grpc_tool.h @@ -19,12 +19,11 @@ #ifndef GRPC_TEST_CPP_UTIL_GRPC_TOOL_H #define GRPC_TEST_CPP_UTIL_GRPC_TOOL_H -#include - #include +#include + #include "test/cpp/util/cli_credentials.h" -#include "test/cpp/util/cli_flags.h" namespace grpc { namespace testing { diff --git a/test/cpp/util/grpc_tool_test.cc b/test/cpp/util/grpc_tool_test.cc index eecef36ca89..c98a7d1d2a8 100644 --- a/test/cpp/util/grpc_tool_test.cc +++ b/test/cpp/util/grpc_tool_test.cc @@ -30,7 +30,6 @@ #include #include -#include #include #include "src/core/lib/gpr/env.h" @@ -55,8 +54,6 @@ using grpc::testing::EchoResponse; "Echo\n" \ "Echo1\n" \ "Echo2\n" \ - "CheckDeadlineUpperBound\n" \ - "CheckDeadlineSet\n" \ "CheckClientInitialMetadata\n" \ "RequestStream\n" \ "ResponseStream\n" \ @@ -73,10 +70,6 @@ using grpc::testing::EchoResponse; "{}\n" \ " rpc Echo2(grpc.testing.EchoRequest) returns (grpc.testing.EchoResponse) " \ "{}\n" \ - " rpc CheckDeadlineUpperBound(grpc.testing.SimpleRequest) returns " \ - "(grpc.testing.StringValue) {}\n" \ - " rpc CheckDeadlineSet(grpc.testing.SimpleRequest) returns " \ - "(grpc.testing.StringValue) {}\n" \ " rpc CheckClientInitialMetadata(grpc.testing.SimpleRequest) returns " \ "(grpc.testing.SimpleResponse) {}\n" \ " rpc RequestStream(stream grpc.testing.EchoRequest) returns " \ @@ -116,6 +109,17 @@ DECLARE_string(ssl_target); namespace grpc { namespace testing { +DECLARE_bool(binary_input); +DECLARE_bool(binary_output); +DECLARE_bool(json_input); +DECLARE_bool(json_output); +DECLARE_bool(l); +DECLARE_bool(batch); +DECLARE_string(metadata); +DECLARE_string(protofiles); +DECLARE_string(proto_path); +DECLARE_string(default_service_config); + namespace { const int kServerDefaultResponseStreamsToSend = 3; @@ -173,29 +177,6 @@ class TestServiceImpl : public ::grpc::testing::EchoTestService::Service { return Status::OK; } - Status CheckDeadlineSet(ServerContext* context, const SimpleRequest* request, - StringValue* response) override { - response->set_message(context->deadline() != - std::chrono::system_clock::time_point::max() - ? "true" - : "false"); - return Status::OK; - } - - // Check if deadline - current time <= timeout - // If deadline set, timeout + current time should be an upper bound for it - Status CheckDeadlineUpperBound(ServerContext* context, - const SimpleRequest* request, - StringValue* response) override { - auto seconds = std::chrono::duration_cast( - context->deadline() - std::chrono::system_clock::now()); - - // Returning string instead of bool to avoid using embedded messages in - // proto3 - response->set_message(seconds.count() <= FLAGS_timeout ? "true" : "false"); - return Status::OK; - } - Status RequestStream(ServerContext* context, ServerReader* reader, EchoResponse* response) override { @@ -881,93 +862,6 @@ TEST_F(GrpcToolTest, CallCommandRequestStreamWithBadRequestJsonInput) { ShutdownServer(); } -TEST_F(GrpcToolTest, CallCommandWithTimeoutDeadlineSet) { - // Test input "grpc_cli call CheckDeadlineSet --timeout=5000.25" - std::stringstream output_stream; - - const std::string server_address = SetUpServer(); - const char* argv[] = {"grpc_cli", "call", server_address.c_str(), - "CheckDeadlineSet"}; - - // Set timeout to 5000.25 seconds - FLAGS_timeout = 5000.25; - - EXPECT_TRUE(0 == GrpcToolMainLib(ArraySize(argv), argv, TestCliCredentials(), - std::bind(PrintStream, &output_stream, - std::placeholders::_1))); - - // Expected output: "message: "true"", deadline set - EXPECT_TRUE(nullptr != - strstr(output_stream.str().c_str(), "message: \"true\"")); - ShutdownServer(); -} - -TEST_F(GrpcToolTest, CallCommandWithTimeoutDeadlineUpperBound) { - // Test input "grpc_cli call CheckDeadlineUpperBound --timeout=900" - std::stringstream output_stream; - - const std::string server_address = SetUpServer(); - const char* argv[] = {"grpc_cli", "call", server_address.c_str(), - "CheckDeadlineUpperBound"}; - - // Set timeout to 900 seconds - FLAGS_timeout = 900; - - EXPECT_TRUE(0 == GrpcToolMainLib(ArraySize(argv), argv, TestCliCredentials(), - std::bind(PrintStream, &output_stream, - std::placeholders::_1))); - - // Expected output: "message: "true"" - // deadline not greater than timeout + current time - EXPECT_TRUE(nullptr != - strstr(output_stream.str().c_str(), "message: \"true\"")); - ShutdownServer(); -} - -TEST_F(GrpcToolTest, CallCommandWithNegativeTimeoutValue) { - // Test input "grpc_cli call CheckDeadlineSet --timeout=-5" - std::stringstream output_stream; - - const std::string server_address = SetUpServer(); - const char* argv[] = {"grpc_cli", "call", server_address.c_str(), - "CheckDeadlineSet"}; - - // Set timeout to -5 (deadline not set) - FLAGS_timeout = -5; - - EXPECT_TRUE(0 == GrpcToolMainLib(ArraySize(argv), argv, TestCliCredentials(), - std::bind(PrintStream, &output_stream, - std::placeholders::_1))); - - // Expected output: "message: "false"", deadline not set - EXPECT_TRUE(nullptr != - strstr(output_stream.str().c_str(), "message: \"false\"")); - - ShutdownServer(); -} - -TEST_F(GrpcToolTest, CallCommandWithDefaultTimeoutValue) { - // Test input "grpc_cli call CheckDeadlineSet --timeout=-1" - std::stringstream output_stream; - - const std::string server_address = SetUpServer(); - const char* argv[] = {"grpc_cli", "call", server_address.c_str(), - "CheckDeadlineSet"}; - - // Set timeout to -1 (default value, deadline not set) - FLAGS_timeout = -1; - - EXPECT_TRUE(0 == GrpcToolMainLib(ArraySize(argv), argv, TestCliCredentials(), - std::bind(PrintStream, &output_stream, - std::placeholders::_1))); - - // Expected output: "message: "false"", deadline not set - EXPECT_TRUE(nullptr != - strstr(output_stream.str().c_str(), "message: \"false\"")); - - ShutdownServer(); -} - TEST_F(GrpcToolTest, CallCommandResponseStream) { // Test input: grpc_cli call localhost: ResponseStream "message: // 'Hello'"