Remove RedisReply.Clear which is conflict with Reset; Add RedisReply.FormatString/Status/Error

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;
     }

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;

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

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;

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;
 };
 

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));