redisclient exists, hmset, hgetallordered function, warm restart table names and table getDbId() function by jipanyang · Pull Request #208 · sonic-net/sonic-swss-common

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 <[email protected]>

pavel-shirshov

as comments

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

common/redisclient.cpp

    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


+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

 #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


+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

    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 <[email protected]>

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

    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 <[email protected]>

Signed-off-by: Qi Luo <[email protected]>

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

    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

    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 <[email protected]> * Fix: implement template function in header filese Signed-off-by: Qi Luo <[email protected]> * Remove unused variable Signed-off-by: Qi Luo <[email protected]> * Implement template hmset Signed-off-by: Qi Luo <[email protected]>

…erits from YCableBase required for Y-Cable API's in sonic-platform-daemons (sonic-net#208) This PR adds support for YCable class required for platform-daemons to use the YCable API's for Broadcom. Description Basically a vendor specific implementation of abstract YCableBase class . detailed design discussion can be found https://github.com/Azure/SONiC/pull/757/files Motivation and Context Required for transitioning to vendor agnostic API's to be called by xcvrd, so that all types of cables can be called. How Has This Been Tested? Ran the changes on Arista7050cx3 switch, making changes inside the container. Signed-off-by: vaibhav-dahiya <[email protected]>

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 <[email protected]>

Add warm restart table names for appDB and configDB

2939e6a

Signed-off-by: Jipan Yang <[email protected]>

Add getDbID() function for tableBase

5fe4e57

Signed-off-by: Jipan Yang <[email protected]>

qiluo-msft requested review from jleveque, pavel-shirshov and qiluo-msft and removed request for jleveque and qiluo-msft 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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

Implement template shmset.formatHMSET()

58dacdd

Signed-off-by: Qi Luo <[email protected]>

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

Conversation

jipanyang commented Jul 17, 2018

Uh oh!

pavel-shirshov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov Jul 17, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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