This is an automated email from the ASF dual-hosted git repository.
jamesge pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-brpc.git
The following commit(s) were added to refs/heads/master by this push:
new a3b2266 Remove RedisReply.Clear which is conflict with Reset; Add
RedisReply.FormatString/Status/Error
a3b2266 is described below
commit a3b22660096c4f00cfdffc7571063603ccb5a428
Author: jamesge <[email protected]>
AuthorDate: Tue Jan 7 19:48:36 2020 +0800
Remove RedisReply.Clear which is conflict with Reset; Add
RedisReply.FormatString/Status/Error
---
example/redis_c++/redis_server.cpp | 12 ++---
src/brpc/redis.cpp | 2 +-
src/brpc/redis_reply.cpp | 25 ++++++++---
src/brpc/redis_reply.h | 41 +++++++++++------
src/butil/string_printf.cpp | 10 ++---
test/brpc_redis_unittest.cpp | 91 ++++++++++++++++++++++----------------
6 files changed, 111 insertions(+), 70 deletions(-)
diff --git a/example/redis_c++/redis_server.cpp
b/example/redis_c++/redis_server.cpp
index ce39de9..abe4d21 100644
--- a/example/redis_c++/redis_server.cpp
+++ b/example/redis_c++/redis_server.cpp
@@ -28,6 +28,8 @@
#include <butil/time.h>
+DEFINE_int32(port, 6379, "TCP Port of this server");
+
class RedisServiceImpl : public brpc::RedisService {
public:
bool Set(const std::string& key, const std::string& value) {
@@ -65,8 +67,8 @@ public:
brpc::RedisCommandHandler::Result Run(const std::vector<const char*>& args,
brpc::RedisReply* output,
bool /*flush_batched*/) override {
- if (args.size() <= 1) {
- output->SetError("ERR wrong number of arguments for 'get'
command");
+ if (args.size() != 2ul) {
+ output->FormatError("Expect 1 arg for 'get', actually %lu",
args.size()-1);
return brpc::RedisCommandHandler::OK;
}
const std::string key(args[1]);
@@ -91,8 +93,8 @@ public:
brpc::RedisCommandHandler::Result Run(const std::vector<const char*>& args,
brpc::RedisReply* output,
bool /*flush_batched*/) override {
- if (args.size() <= 2) {
- output->SetError("ERR wrong number of arguments for 'set'
command");
+ if (args.size() != 3ul) {
+ output->FormatError("Expect 2 args for 'set', actually %lu",
args.size()-1);
return brpc::RedisCommandHandler::OK;
}
const std::string key(args[1]);
@@ -115,7 +117,7 @@ int main(int argc, char* argv[]) {
brpc::Server server;
brpc::ServerOptions server_options;
server_options.redis_service = rsimpl;
- if (server.Start(6379, &server_options) != 0) {
+ if (server.Start(FLAGS_port, &server_options) != 0) {
LOG(ERROR) << "Fail to start server";
return -1;
}
diff --git a/src/brpc/redis.cpp b/src/brpc/redis.cpp
index 267b461..7cfc21f 100644
--- a/src/brpc/redis.cpp
+++ b/src/brpc/redis.cpp
@@ -272,7 +272,7 @@ RedisResponse* RedisResponse::New() const {
}
void RedisResponse::Clear() {
- _first_reply.Clear();
+ _first_reply.Reset();
_other_replies = NULL;
_arena.clear();
_nreply = 0;
diff --git a/src/brpc/redis_reply.cpp b/src/brpc/redis_reply.cpp
index 5d67477..8710df1 100644
--- a/src/brpc/redis_reply.cpp
+++ b/src/brpc/redis_reply.cpp
@@ -409,9 +409,6 @@ void RedisReply::CopyFromDifferentArena(const RedisReply&
other) {
}
void RedisReply::SetArray(int size) {
- if (!_arena) {
- return;
- }
if (_type != REDIS_REPLY_NIL) {
Reset();
}
@@ -436,9 +433,6 @@ void RedisReply::SetArray(int size) {
}
void RedisReply::SetStringImpl(const butil::StringPiece& str, RedisReplyType
type) {
- if (!_arena) {
- return;
- }
if (_type != REDIS_REPLY_NIL) {
Reset();
}
@@ -460,4 +454,23 @@ void RedisReply::SetStringImpl(const butil::StringPiece&
str, RedisReplyType typ
_length = size;
}
+void RedisReply::FormatStringImpl(const char* fmt, va_list args,
RedisReplyType type) {
+ va_list copied_args;
+ va_copy(copied_args, args);
+ char buf[64];
+ int ret = vsnprintf(buf, sizeof(buf), fmt, copied_args);
+ va_end(copied_args);
+ if (ret < 0) {
+ LOG(FATAL) << "Fail to vsnprintf into buf=" << (void*)buf << " size="
<< sizeof(buf);
+ return;
+ } else if (ret < (int)sizeof(buf)) {
+ return SetStringImpl(buf, type);
+ } else {
+ std::string str;
+ str.reserve(ret + 1);
+ butil::string_vappendf(&str, fmt, args);
+ return SetStringImpl(str, type);
+ }
+}
+
} // namespace brpc
diff --git a/src/brpc/redis_reply.h b/src/brpc/redis_reply.h
index ec26915..d2515d8 100644
--- a/src/brpc/redis_reply.h
+++ b/src/brpc/redis_reply.h
@@ -67,17 +67,20 @@ public:
// value.
void SetArray(int size);
- // Set the reply to status message `str'.
+ // Set the reply to a status.
void SetStatus(const butil::StringPiece& str);
+ void FormatStatus(const char* fmt, ...);
- // Set the reply to error message `str'.
+ // Set the reply to an error.
void SetError(const butil::StringPiece& str);
+ void FormatError(const char* fmt, ...);
- // Set the reply to integer `value'.
+ // Set this reply to integer `value'.
void SetInteger(int64_t value);
- // Set the reply to string `str'.
+ // Set this reply to a (bulk) string.
void SetString(const butil::StringPiece& str);
+ void FormatString(const char* fmt, ...);
// Convert the reply into a signed 64-bit integer(according to
// http://redis.io/topics/protocol). If the reply is not an integer,
@@ -125,7 +128,7 @@ public:
void Swap(RedisReply& other);
// Reset to the state that this reply was just constructed.
- void Clear();
+ void Reset();
// Print fields into ostream
void Print(std::ostream& os) const;
@@ -143,8 +146,8 @@ private:
// by calling CopyFrom[Different|Same]Arena.
DISALLOW_COPY_AND_ASSIGN(RedisReply);
+ void FormatStringImpl(const char* fmt, va_list args, RedisReplyType type);
void SetStringImpl(const butil::StringPiece& str, RedisReplyType type);
- void Reset();
RedisReplyType _type;
int _length; // length of short_str/long_str, count of replies
@@ -218,10 +221,22 @@ inline void RedisReply::SetNullString() {
inline void RedisReply::SetStatus(const butil::StringPiece& str) {
return SetStringImpl(str, REDIS_REPLY_STATUS);
}
+inline void RedisReply::FormatStatus(const char* fmt, ...) {
+ va_list ap;
+ va_start(ap, fmt);
+ FormatStringImpl(fmt, ap, REDIS_REPLY_STATUS);
+ va_end(ap);
+}
inline void RedisReply::SetError(const butil::StringPiece& str) {
return SetStringImpl(str, REDIS_REPLY_ERROR);
}
+inline void RedisReply::FormatError(const char* fmt, ...) {
+ va_list ap;
+ va_start(ap, fmt);
+ FormatStringImpl(fmt, ap, REDIS_REPLY_ERROR);
+ va_end(ap);
+}
inline void RedisReply::SetInteger(int64_t value) {
if (_type != REDIS_REPLY_NIL) {
@@ -235,6 +250,12 @@ inline void RedisReply::SetInteger(int64_t value) {
inline void RedisReply::SetString(const butil::StringPiece& str) {
return SetStringImpl(str, REDIS_REPLY_STRING);
}
+inline void RedisReply::FormatString(const char* fmt, ...) {
+ va_list ap;
+ va_start(ap, fmt);
+ FormatStringImpl(fmt, ap, REDIS_REPLY_STRING);
+ va_end(ap);
+}
inline const char* RedisReply::c_str() const {
if (is_string()) {
@@ -300,14 +321,6 @@ inline void RedisReply::Swap(RedisReply& other) {
std::swap(_arena, other._arena);
}
-inline void RedisReply::Clear() {
- _type = REDIS_REPLY_NIL;
- _length = 0;
- _data.array.last_index = -1;
- _data.array.replies = NULL;
- // _arena should not be cleared because it may be shared between
RedisReply;
-}
-
inline void RedisReply::CopyFromSameArena(const RedisReply& other) {
_type = other._type;
_length = other._length;
diff --git a/src/butil/string_printf.cpp b/src/butil/string_printf.cpp
index 25469f1..981420e 100644
--- a/src/butil/string_printf.cpp
+++ b/src/butil/string_printf.cpp
@@ -108,10 +108,9 @@ int string_appendf(std::string* output, const char*
format, ...) {
int string_vappendf(std::string* output, const char* format, va_list args) {
const size_t old_size = output->size();
const int rc = string_printf_impl(*output, format, args);
- if (rc == 0) {
- return 0;
+ if (rc != 0) {
+ output->resize(old_size);
}
- output->resize(old_size);
return rc;
}
@@ -130,10 +129,9 @@ int string_printf(std::string* output, const char* format,
...) {
int string_vprintf(std::string* output, const char* format, va_list args) {
output->clear();
const int rc = string_printf_impl(*output, format, args);
- if (rc == 0) {
- return 0;
+ if (rc != 0) {
+ output->clear();
}
- output->clear();
return rc;
};
diff --git a/test/brpc_redis_unittest.cpp b/test/brpc_redis_unittest.cpp
index 0152f63..c9d4b0d 100644
--- a/test/brpc_redis_unittest.cpp
+++ b/test/brpc_redis_unittest.cpp
@@ -643,11 +643,12 @@ TEST_F(RedisTest, redis_reply_codec) {
appender.move_to(buf);
ASSERT_STREQ(buf.to_string().c_str(), "+OK\r\n");
ASSERT_STREQ(r.c_str(), "OK");
- r.Clear();
- brpc::ParseError err = r.ConsumePartialIOBuf(buf);
+
+ brpc::RedisReply r2(&arena);
+ brpc::ParseError err = r2.ConsumePartialIOBuf(buf);
ASSERT_EQ(err, brpc::PARSE_OK);
- ASSERT_TRUE(r.is_string());
- ASSERT_STREQ("OK", r.c_str());
+ ASSERT_TRUE(r2.is_string());
+ ASSERT_STREQ("OK", r2.c_str());
}
// error
{
@@ -658,11 +659,12 @@ TEST_F(RedisTest, redis_reply_codec) {
ASSERT_TRUE(r.SerializeTo(&appender));
appender.move_to(buf);
ASSERT_STREQ(buf.to_string().c_str(), "-not exist \'key\'\r\n");
- r.Clear();
- brpc::ParseError err = r.ConsumePartialIOBuf(buf);
+
+ brpc::RedisReply r2(&arena);
+ brpc::ParseError err = r2.ConsumePartialIOBuf(buf);
ASSERT_EQ(err, brpc::PARSE_OK);
- ASSERT_TRUE(r.is_error());
- ASSERT_STREQ("not exist \'key\'", r.error_message());
+ ASSERT_TRUE(r2.is_error());
+ ASSERT_STREQ("not exist \'key\'", r2.error_message());
}
// string
{
@@ -673,22 +675,35 @@ TEST_F(RedisTest, redis_reply_codec) {
ASSERT_TRUE(r.SerializeTo(&appender));
appender.move_to(buf);
ASSERT_STREQ(buf.to_string().c_str(), "$-1\r\n");
- r.Clear();
- brpc::ParseError err = r.ConsumePartialIOBuf(buf);
+
+ brpc::RedisReply r2(&arena);
+ brpc::ParseError err = r2.ConsumePartialIOBuf(buf);
ASSERT_EQ(err, brpc::PARSE_OK);
- ASSERT_TRUE(r.is_nil());
+ ASSERT_TRUE(r2.is_nil());
- r.Clear();
r.SetString("abcde'hello world");
ASSERT_TRUE(r.SerializeTo(&appender));
appender.move_to(buf);
ASSERT_STREQ(buf.to_string().c_str(), "$17\r\nabcde'hello world\r\n");
- ASSERT_STREQ(r.c_str(), "abcde'hello world");
- r.Clear();
- err = r.ConsumePartialIOBuf(buf);
+ ASSERT_STREQ("abcde'hello world", r.c_str());
+
+ r.FormatString("int:%d str:%s fp:%.2f", 123, "foobar", 3.21);
+ ASSERT_TRUE(r.SerializeTo(&appender));
+ appender.move_to(buf);
+ ASSERT_STREQ(buf.to_string().c_str(), "$26\r\nint:123 str:foobar
fp:3.21\r\n");
+ ASSERT_STREQ("int:123 str:foobar fp:3.21", r.c_str());
+
+ r.FormatString("verylongstring verylongstring verylongstring
verylongstring int:%d str:%s fp:%.2f", 123, "foobar", 3.21);
+ ASSERT_TRUE(r.SerializeTo(&appender));
+ appender.move_to(buf);
+ ASSERT_STREQ(buf.to_string().c_str(), "$86\r\nverylongstring
verylongstring verylongstring verylongstring int:123 str:foobar fp:3.21\r\n");
+ ASSERT_STREQ("verylongstring verylongstring verylongstring
verylongstring int:123 str:foobar fp:3.21", r.c_str());
+
+ brpc::RedisReply r3(&arena);
+ err = r3.ConsumePartialIOBuf(buf);
ASSERT_EQ(err, brpc::PARSE_OK);
- ASSERT_TRUE(r.is_string());
- ASSERT_STREQ(r.c_str(), "abcde'hello world");
+ ASSERT_TRUE(r3.is_string());
+ ASSERT_STREQ(r.c_str(), r3.c_str());
}
// integer
{
@@ -699,16 +714,16 @@ TEST_F(RedisTest, redis_reply_codec) {
int input[] = { -1, 1234567 };
const char* output[] = { ":-1\r\n", ":1234567\r\n" };
for (int i = 0; i < t; ++i) {
- r.Clear();
r.SetInteger(input[i]);
ASSERT_TRUE(r.SerializeTo(&appender));
appender.move_to(buf);
ASSERT_STREQ(buf.to_string().c_str(), output[i]);
- r.Clear();
- brpc::ParseError err = r.ConsumePartialIOBuf(buf);
+
+ brpc::RedisReply r2(&arena);
+ brpc::ParseError err = r2.ConsumePartialIOBuf(buf);
ASSERT_EQ(err, brpc::PARSE_OK);
- ASSERT_TRUE(r.is_integer());
- ASSERT_EQ(r.integer(), input[i]);
+ ASSERT_TRUE(r2.is_integer());
+ ASSERT_EQ(r2.integer(), input[i]);
}
}
// array
@@ -729,22 +744,22 @@ TEST_F(RedisTest, redis_reply_codec) {
ASSERT_STREQ(buf.to_string().c_str(),
"*3\r\n*2\r\n$14\r\nhello, it's me\r\n:422\r\n$21\r\n"
"To go over everything\r\n:1\r\n");
- r.Clear();
- ASSERT_EQ(r.ConsumePartialIOBuf(buf), brpc::PARSE_OK);
- ASSERT_TRUE(r.is_array());
- ASSERT_EQ(3ul, r.size());
- ASSERT_TRUE(r[0].is_array());
- ASSERT_EQ(2ul, r[0].size());
- ASSERT_TRUE(r[0][0].is_string());
- ASSERT_STREQ(r[0][0].c_str(), "hello, it's me");
- ASSERT_TRUE(r[0][1].is_integer());
- ASSERT_EQ(r[0][1].integer(), 422);
- ASSERT_TRUE(r[1].is_string());
- ASSERT_STREQ(r[1].c_str(), "To go over everything");
- ASSERT_TRUE(r[2].is_integer());
- ASSERT_EQ(1, r[2].integer());
-
- r.Clear();
+
+ brpc::RedisReply r2(&arena);
+ ASSERT_EQ(r2.ConsumePartialIOBuf(buf), brpc::PARSE_OK);
+ ASSERT_TRUE(r2.is_array());
+ ASSERT_EQ(3ul, r2.size());
+ ASSERT_TRUE(r2[0].is_array());
+ ASSERT_EQ(2ul, r2[0].size());
+ ASSERT_TRUE(r2[0][0].is_string());
+ ASSERT_STREQ(r2[0][0].c_str(), "hello, it's me");
+ ASSERT_TRUE(r2[0][1].is_integer());
+ ASSERT_EQ(r2[0][1].integer(), 422);
+ ASSERT_TRUE(r2[1].is_string());
+ ASSERT_STREQ(r2[1].c_str(), "To go over everything");
+ ASSERT_TRUE(r2[2].is_integer());
+ ASSERT_EQ(1, r2[2].integer());
+
// null array
r.SetNullArray();
ASSERT_TRUE(r.SerializeTo(&appender));
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
