redisclient exists, hmset, hgetallordered function, warm restart table names and table getDbId() function #208

jipanyang · 2018-07-17T06:57:01Z

This is to add some swss common function support for warm reboot development:

<1> redisclient exists(), hmset(), hgetallordered() functions.
<2> warm restart table names in appDB and configDB
<3> getDbId() function for Table class

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

pavel-shirshov

as comments

pavel-shirshov · 2018-07-17T19:33:21Z

common/redisclient.cpp

@@ -20,6 +20,14 @@ int64_t RedisClient::del(const string &key)
    return r.getContext()->integer;
 }

+int64_t RedisClient::exists(const string &key)


I think it's better to use bool as return value

int64_t is the return value type from libhiredis for exists call.
We are giving one key as parameter here, using bool as return value seems reasonable. But is it a concern if we deviate from libhiredis as to the return value type?

Hi Jipan,
I got your point, but I think in case of this library we have abstracted hiredis library and use swss-common instead. EXISTS returns 0 or 1 with Boolean semantics. So I think it's better to make it explicitly Boolean.
When I initially saw that your method return int64_t I checked redis documentation, because it was not clear for me why we need int64_t to return exists result. #Closed

Ok, will change to bool return.

https://redis.io/commands/exists
Because redis command return integer. However, we don't need it.

In reply to: 203180025 [](ancestors = 203180025)

pavel-shirshov · 2018-07-17T19:34:22Z

common/redisclient.cpp

+int64_t RedisClient::exists(const string &key)
+{
+    RedisCommand sexists;
+    sexists.format("EXISTS %s", key.c_str());


I think It would be faster to concatenate to strings than use .format her.

.format is the redis library function, it is being used in almost all the exist swss-common redis related functions. I assume it should be fast and safe enough?

Agree with you. But I think concatenating two strings are faster than parse a format string.
So it's more a speed optimization.

After checking it again, RedisCommand is swss-common class, format is the member function of it which in turn calls redisvFormatCommand() to populate the command data.

I saw comments like "Build the command string accordingly to protocol" in redisvFormatCommand(), probably it is safer to use format() here to avoid any potential issue.

pavel-shirshov · 2018-07-17T19:35:13Z

common/redisclient.cpp

+    auto it = vmap.begin();
+    while (it != vmap.end())
+    {
+        values.push_back(FieldValueTuple(it->first, it->second));


,emplace_back(it->first, it->second)

pavel-shirshov · 2018-07-17T19:37:04Z

common/redisclient.cpp

+
+    vector<FieldValueTuple> values;
+    auto it = vmap.begin();
+    while (it != vmap.end())


is probably better to use
for(const auto it: vmap) ?

pavel-shirshov · 2018-07-17T19:38:50Z

common/redisclient.cpp

+std::map<std::string, std::string> RedisClient::hgetallordered(const std::string &key)
+{
+    RedisCommand sincr;
+    sincr.format("HGETALL %s", key.c_str());


use string concatenation instead of format?

pavel-shirshov · 2018-07-17T19:40:05Z

common/redisclient.cpp

+
+    map<string, string> map;
+    for (unsigned int i = 0; i < ctx->elements; i += 2)
+        map[string(ctx->element[i]->str)] = string(ctx->element[i+1]->str);


map.emplace(ctx->element[i]->str, ctx->element[i+1]->str) ?

pavel-shirshov · 2018-07-17T19:41:49Z

common/redisclient.cpp

@@ -35,6 +43,34 @@ void RedisClient::hset(const string &key, const string &field, const string &val
    RedisReply r(m_db, shset, REDIS_REPLY_INTEGER);
 }

+void RedisClient::hmset(const string &key, const vector<FieldValueTuple> &values)
+{
+    if (values.size() == 0)


if (values.empty()) ?

pavel-shirshov · 2018-07-17T19:42:11Z

common/redisclient.cpp

+
+void RedisClient::hmset(const string &key, const std::map<std::string, std::string> &vmap)
+{
+    if (vmap.size() == 0)


if (vmap.empty())?

qiluo-msft · 2018-07-17T21:02:43Z

common/schema.h

@@ -13,6 +13,7 @@ namespace swss {
 #define PFC_WD_DB       5
 #define FLEX_COUNTER_DB 5
 #define STATE_DB        6
+#define RESTORE_DB      7


RESTORE_DB [](start = 8, length = 10)

Let's postpone schema changes into future PR. #Closed

Sorry, wrong push. Should be gone now.

qiluo-msft · 2018-07-17T21:22:40Z

common/redisclient.cpp

@@ -20,6 +20,14 @@ int64_t RedisClient::del(const string &key)
    return r.getContext()->integer;
 }

+int64_t RedisClient::exists(const string &key)
+{
+    RedisCommand sexists;


sexists [](start = 17, length = 7)

Let's use a different var name. #Closed

qiluo-msft · 2018-07-17T21:34:54Z

common/redisclient.cpp

@@ -20,6 +20,14 @@ int64_t RedisClient::del(const string &key)
    return r.getContext()->integer;
 }

+int64_t RedisClient::exists(const string &key)


key [](start = 42, length = 3)

If the key contains any blank characters (including \t), there will be problem ignored. #Closed

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

qiluo-msft · 2018-07-17T21:49:43Z

common/redisclient.cpp

+{
+    if (values.size() == 0)
+        return;
+


You don't need this check. Already checked in RedisCommand::formatHMSET() #Closed

Ok, will remote the check in hmset()

qiluo-msft · 2018-07-17T21:51:23Z

common/redisclient.cpp

+    RedisReply r(m_db, shmset, REDIS_REPLY_STATUS);
+}
+
+void RedisClient::hmset(const string &key, const std::map<std::string, std::string> &vmap)


RedisClient::hmset [](start = 5, length = 18)

You can overload formatHMSET(), and call it. #Closed

Not sure what change you mean here?

You may define another RedisCommand::formatHMSET() function, and call it here. #Closed

I will provide a PR.

In reply to: 203206068 [](ancestors = 203206068)

qiluo-msft · 2018-07-17T21:55:01Z

common/redisclient.cpp

@@ -57,6 +93,21 @@ unordered_map<string, string> RedisClient::hgetall(const string &key)
    return map;
 }

+std::map<std::string, std::string> RedisClient::hgetallordered(const std::string &key)


hgetallordered [](start = 48, length = 14)

Redis hash fields have no order. What is the use case? #Closed

We have case to sort the field in string alphabetical order, instead of getting data into unordered_map<string, string> then sort them again outside, it is more efficient putting data directly into map<string, string>. #Closed

This is a valid use case, however, it makes no sense to implement for every possible type of output container. And redis hash fields has no order by design.

You may implement a general function with inserter. Sample: http://www.cplusplus.com/forum/general/3194/

In reply to: 203201493 [](ancestors = 203201493)

I will provide a PR based on your PR.

In reply to: 203208567 [](ancestors = 203208567,203201493)

qiluo-msft

As comments.

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>

Help resolve review comments

pavel-shirshov · 2018-07-20T18:09:42Z

common/redisclient.cpp

+bool RedisClient::exists(const string &key)
+{
+    RedisCommand rexists;
+    if (key.find_first_of(" \t") != string::npos)


Probably it's better to trim spaces and tabs from the key? #Resolved

Actually this input validation is to prevent any input like "key1 key2". Not for conveniently trimming.

In reply to: 204127076 [](ancestors = 204127076)

I'm good. Thank you for the explanations. I'd a comment explaining why do we need this validation here.

pavel-shirshov · 2018-07-20T18:11:28Z

common/redisclient.cpp

@@ -35,6 +48,20 @@ void RedisClient::hset(const string &key, const string &field, const string &val
    RedisReply r(m_db, shset, REDIS_REPLY_INTEGER);
 }

+void RedisClient::hmset(const string &key, const vector<FieldValueTuple> &values)


implement both functions as a template? The code in them are identical #Resolved

Agree. I will follow up.

In reply to: 204127551 [](ancestors = 204127551)

pavel-shirshov · 2018-07-20T18:12:40Z

common/redisclient.cpp

 {
    RedisCommand sincr;
    sincr.format("HGETALL %s", key.c_str());
    RedisReply r(m_db, sincr, REDIS_REPLY_ARRAY);

    auto ctx = r.getContext();

-    unordered_map<string, string> map;
+    map<string, string> map;


what it was changed to map? Do we need an order here? #Resolved

It is not used and should be removed. My mistake and I will fix it.

In reply to: 204127846 [](ancestors = 204127846)

* Fix swig compile Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com> * Fix: implement template function in header filese Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com> * Remove unused variable Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com> * Implement template hmset Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>

jipanyang added 4 commits July 5, 2018 09:15

Add hmset and hgetallordered redis api function support

151be00

Add exists command support for redis client

77352f6

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

Add warm restart table names for appDB and configDB

2939e6a

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

Add getDbID() function for tableBase

5fe4e57

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

qiluo-msft requested review from qiluo-msft, jleveque and pavel-shirshov and removed request for qiluo-msft and jleveque July 17, 2018 19:21

qiluo-msft assigned lguohan and unassigned lguohan Jul 17, 2018

qiluo-msft requested a review from lguohan July 17, 2018 19:22

pavel-shirshov suggested changes Jul 17, 2018

View reviewed changes

qiluo-msft reviewed Jul 17, 2018

View reviewed changes

Address review comments

c1414ed

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

qiluo-msft reviewed Jul 17, 2018

View reviewed changes

qiluo-msft requested changes Jul 17, 2018

View reviewed changes

Remove redundant empty check

bbff96a

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

pavel-shirshov approved these changes Jul 17, 2018

View reviewed changes

For RedisClient exists, make sure no space or tab in key

b1f270e

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

jipanyang force-pushed the warm_reboot_collab branch from 9ff098a to b1f270e Compare July 20, 2018 00:01

qiluo-msft and others added 3 commits July 20, 2018 01:55

Implement template hgetall

dc7f42d

Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>

Implement template shmset.formatHMSET()

58dacdd

Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>

Merge pull request #2 from qiluo-msft/jipan/warm_reboot_collab

876a991

Help resolve review comments

pavel-shirshov reviewed Jul 20, 2018

View reviewed changes

qiluo-msft approved these changes Jul 20, 2018

View reviewed changes

qiluo-msft merged commit 9fa06da into sonic-net:master Jul 20, 2018

redisclient exists, hmset, hgetallordered function, warm restart table names and table getDbId() function #208

redisclient exists, hmset, hgetallordered function, warm restart table names and table getDbId() function #208

Conversation

jipanyang commented Jul 17, 2018

pavel-shirshov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavel-shirshov Jul 17, 2018 • edited by qiluo-msft Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

jipanyang Jul 17, 2018 • edited by qiluo-msft Loading

Choose a reason for hiding this comment

qiluo-msft Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft left a comment

Choose a reason for hiding this comment

pavel-shirshov Jul 20, 2018 • edited by qiluo-msft Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavel-shirshov Jul 20, 2018 • edited by qiluo-msft Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavel-shirshov Jul 20, 2018 • edited by qiluo-msft Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavel-shirshov Jul 17, 2018 •

edited by qiluo-msft

Loading

qiluo-msft Jul 17, 2018 •

edited

Loading

qiluo-msft Jul 17, 2018 •

edited

Loading

qiluo-msft Jul 17, 2018 •

edited

Loading

qiluo-msft Jul 17, 2018 •

edited

Loading

qiluo-msft Jul 17, 2018 •

edited

Loading

qiluo-msft Jul 17, 2018 •

edited

Loading

qiluo-msft Jul 17, 2018 •

edited

Loading

jipanyang Jul 17, 2018 •

edited by qiluo-msft

Loading

qiluo-msft Jul 17, 2018 •

edited

Loading

pavel-shirshov Jul 20, 2018 •

edited by qiluo-msft

Loading

pavel-shirshov Jul 20, 2018 •

edited by qiluo-msft

Loading

pavel-shirshov Jul 20, 2018 •

edited by qiluo-msft

Loading