You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by to...@apache.org on 2016/05/04 00:37:07 UTC
[2/2] incubator-kudu git commit: rpc: improve error messages for
overly long RPC responses
rpc: improve error messages for overly long RPC responses
This improves the error messages to include the type name of the
PB, and also adds test coverage.
Change-Id: Id03f2e76d5929bfee33d3b2e4b55c74a97f95254
Reviewed-on: http://gerrit.cloudera.org:8080/2915
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/incubator-kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-kudu/commit/d3eb2d8f
Tree: http://git-wip-us.apache.org/repos/asf/incubator-kudu/tree/d3eb2d8f
Diff: http://git-wip-us.apache.org/repos/asf/incubator-kudu/diff/d3eb2d8f
Branch: refs/heads/master
Commit: d3eb2d8f44a6e2e8cc9e6c8c01955eacba4ca0f3
Parents: b688333
Author: Todd Lipcon <to...@apache.org>
Authored: Sat Apr 30 14:17:23 2016 -0700
Committer: Todd Lipcon <to...@apache.org>
Committed: Tue May 3 22:36:38 2016 +0000
----------------------------------------------------------------------
src/kudu/rpc/rpc-test-base.h | 4 ++++
src/kudu/rpc/rpc_stub-test.cc | 14 ++++++++++++++
src/kudu/rpc/rtest.proto | 1 +
src/kudu/rpc/serialization.cc | 8 ++++++--
src/kudu/rpc/transfer.cc | 14 +++++++-------
5 files changed, 32 insertions(+), 9 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/d3eb2d8f/src/kudu/rpc/rpc-test-base.h
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/rpc-test-base.h b/src/kudu/rpc/rpc-test-base.h
index db174eb..b0afdbe 100644
--- a/src/kudu/rpc/rpc-test-base.h
+++ b/src/kudu/rpc/rpc-test-base.h
@@ -240,6 +240,10 @@ class CalculatorService : public CalculatorServiceIf {
// marked as required. This exercises the error path of invalid responses.
context->RespondSuccess();
break;
+ case rpc_test::TestInvalidResponseRequestPB_ErrorType_RESPONSE_TOO_LARGE:
+ resp->mutable_response()->resize(FLAGS_rpc_max_message_size + 1000);
+ context->RespondSuccess();
+ break;
default:
LOG(FATAL);
}
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/d3eb2d8f/src/kudu/rpc/rpc_stub-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/rpc_stub-test.cc b/src/kudu/rpc/rpc_stub-test.cc
index d23d6ee..770017a 100644
--- a/src/kudu/rpc/rpc_stub-test.cc
+++ b/src/kudu/rpc/rpc_stub-test.cc
@@ -244,6 +244,20 @@ TEST_F(RpcStubTest, TestResponseWithMissingField) {
"invalid RPC response, missing fields: response");
}
+// Test case where the server responds with a message which is larger than the maximum
+// configured RPC message size. The server should send the response, but the client
+// will reject it.
+TEST_F(RpcStubTest, TestResponseLargerThanFrameSize) {
+ CalculatorServiceProxy p(client_messenger_, server_addr_);
+
+ RpcController rpc;
+ TestInvalidResponseRequestPB req;
+ TestInvalidResponseResponsePB resp;
+ req.set_error_type(rpc_test::TestInvalidResponseRequestPB_ErrorType_RESPONSE_TOO_LARGE);
+ Status s = p.TestInvalidResponse(req, &resp, &rpc);
+ ASSERT_STR_CONTAINS(s.ToString(), "Network error: RPC frame had a length of");
+}
+
// Test sending a call which isn't implemented by the server.
TEST_F(RpcStubTest, TestCallMissingMethod) {
Proxy p(client_messenger_, server_addr_, CalculatorService::static_service_name());
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/d3eb2d8f/src/kudu/rpc/rtest.proto
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/rtest.proto b/src/kudu/rpc/rtest.proto
index 076d2f6..be6fc30 100644
--- a/src/kudu/rpc/rtest.proto
+++ b/src/kudu/rpc/rtest.proto
@@ -93,6 +93,7 @@ message PanicResponsePB {}
message TestInvalidResponseRequestPB {
enum ErrorType {
MISSING_REQUIRED_FIELD = 1;
+ RESPONSE_TOO_LARGE = 2;
}
required ErrorType error_type = 1;
}
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/d3eb2d8f/src/kudu/rpc/serialization.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/serialization.cc b/src/kudu/rpc/serialization.cc
index 3ad46bc..c4248cc 100644
--- a/src/kudu/rpc/serialization.cc
+++ b/src/kudu/rpc/serialization.cc
@@ -22,6 +22,7 @@
#include <google/protobuf/io/coded_stream.h>
#include "kudu/gutil/endian.h"
+#include "kudu/gutil/strings/substitute.h"
#include "kudu/gutil/stringprintf.h"
#include "kudu/rpc/constants.h"
#include "kudu/util/faststring.h"
@@ -33,6 +34,7 @@ DECLARE_int32(rpc_max_message_size);
using google::protobuf::MessageLite;
using google::protobuf::io::CodedInputStream;
using google::protobuf::io::CodedOutputStream;
+using strings::Substitute;
namespace kudu {
namespace rpc {
@@ -53,8 +55,10 @@ void SerializeMessage(const MessageLite& message, faststring* param_buf,
int total_size = size_with_delim + additional_size;
if (total_size > FLAGS_rpc_max_message_size) {
- LOG(WARNING) << "Sending too long of an RPC message (" << total_size
- << " bytes)";
+ LOG(WARNING) << Substitute("Serialized $0 ($1 bytes) is larger than the maximum configured "
+ "RPC message size ($2 bytes). "
+ "Sending anyway, but peer may reject the data.",
+ message.GetTypeName(), total_size, FLAGS_rpc_max_message_size);
}
param_buf->resize(size_with_delim);
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/d3eb2d8f/src/kudu/rpc/transfer.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/transfer.cc b/src/kudu/rpc/transfer.cc
index a105ad6..de73f58 100644
--- a/src/kudu/rpc/transfer.cc
+++ b/src/kudu/rpc/transfer.cc
@@ -25,7 +25,6 @@
#include <glog/logging.h>
#include "kudu/gutil/endian.h"
-#include "kudu/gutil/stringprintf.h"
#include "kudu/gutil/strings/substitute.h"
#include "kudu/rpc/constants.h"
#include "kudu/rpc/messenger.h"
@@ -55,6 +54,7 @@ namespace rpc {
using std::ostringstream;
using std::set;
using std::string;
+using strings::Substitute;
#define RETURN_ON_ERROR_OR_SOCKET_NOT_READY(status) \
if (PREDICT_FALSE(!status.ok())) { \
@@ -98,13 +98,13 @@ Status InboundTransfer::ReceiveBuffer(Socket &socket) {
// add that back in.
total_length_ = NetworkByteOrder::Load32(&buf_[0]) + kMsgLengthPrefixLength;
if (total_length_ > FLAGS_rpc_max_message_size) {
- return Status::NetworkError(StringPrintf("the frame had a "
- "length of %d, but we only support messages up to %d bytes "
- "long.", total_length_, FLAGS_rpc_max_message_size));
+ return Status::NetworkError(Substitute(
+ "RPC frame had a length of $0, but we only support messages up to $1 bytes "
+ "long.", total_length_, FLAGS_rpc_max_message_size));
}
if (total_length_ <= kMsgLengthPrefixLength) {
- return Status::NetworkError(StringPrintf("the frame had a "
- "length of %d, which is invalid", total_length_));
+ return Status::NetworkError(Substitute("RPC frame had invalid length of $0",
+ total_length_));
}
buf_.resize(total_length_);
@@ -131,7 +131,7 @@ bool InboundTransfer::TransferFinished() const {
}
string InboundTransfer::StatusAsString() const {
- return strings::Substitute("$0/$1 bytes received", cur_offset_, total_length_);
+ return Substitute("$0/$1 bytes received", cur_offset_, total_length_);
}
OutboundTransfer::OutboundTransfer(int32_t call_id,