Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
qiluo-msft merged 11 commits into from
Jul 20, 2018
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
151be00
Add hmset and hgetallordered redis api function support
jipanyang May 10, 2018
77352f6
Add exists command support for redis client
jipanyang May 29, 2018
2939e6a
Add warm restart table names for appDB and configDB
jipanyang Jul 6, 2018
5fe4e57
Add getDbID() function for tableBase
jipanyang Jul 14, 2018
c1414ed
Address review comments
jipanyang Jul 17, 2018
bbff96a
Remove redundant empty check
jipanyang Jul 17, 2018
b1f270e
For RedisClient exists, make sure no space or tab in key
jipanyang Jul 19, 2018
dc7f42d
Implement template hgetall
qiluo-msft Jul 20, 2018
58dacdd
Implement template shmset.formatHMSET()
qiluo-msft Jul 20, 2018
876a991
Merge pull request #2 from qiluo-msft/jipan/warm_reboot_collab
jipanyang Jul 20, 2018
0d24fd3
Fix build (#3)
qiluo-msft Jul 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 52 additions & 1 deletion common/redisclient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ int64_t RedisClient::del(const string &key)
return r.getContext()->integer;
}

int64_t RedisClient::exists(const string &key)
Copy link
Copy Markdown
Contributor

@pavel-shirshov pavel-shirshov Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will change to bool return.

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jul 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

{
RedisCommand sexists;
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jul 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Let's use a different var name. #Closed

sexists.format("EXISTS %s", key.c_str());
Copy link
Copy Markdown
Contributor

@pavel-shirshov pavel-shirshov Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.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?

Copy link
Copy Markdown
Contributor

@pavel-shirshov pavel-shirshov Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

RedisReply r(m_db, sexists, REDIS_REPLY_INTEGER);
return r.getContext()->integer;
}

int64_t RedisClient::hdel(const string &key, const string &field)
{
RedisCommand shdel;
Expand All @@ -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)
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. I will follow up.

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

{
if (values.size() == 0)
Copy link
Copy Markdown
Contributor

@pavel-shirshov pavel-shirshov Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (values.empty()) ?

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

return;

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jul 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will remote the check in hmset()

RedisCommand shmset;
shmset.formatHMSET(key, values);
RedisReply r(m_db, shmset, REDIS_REPLY_STATUS);
}

void RedisClient::hmset(const string &key, const std::map<std::string, std::string> &vmap)
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jul 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what change you mean here?

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jul 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will provide a PR.

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

{
if (vmap.size() == 0)
Copy link
Copy Markdown
Contributor

@pavel-shirshov pavel-shirshov Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (vmap.empty())?

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

return;

vector<FieldValueTuple> values;
auto it = vmap.begin();
while (it != vmap.end())
Copy link
Copy Markdown
Contributor

@pavel-shirshov pavel-shirshov Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.

{
values.push_back(FieldValueTuple(it->first, it->second));
Copy link
Copy Markdown
Contributor

@pavel-shirshov pavel-shirshov Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

it++;
}

RedisCommand shmset;
shmset.formatHMSET(key, values);
RedisReply r(m_db, shmset, REDIS_REPLY_STATUS);
}

void RedisClient::set(const string &key, const string &value)
{
RedisCommand sset;
Expand All @@ -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)
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jul 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown
Contributor Author

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jul 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will provide a PR based on your PR.

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

{
RedisCommand sincr;
sincr.format("HGETALL %s", key.c_str());
Copy link
Copy Markdown
Contributor

@pavel-shirshov pavel-shirshov Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use string concatenation instead of format?

RedisReply r(m_db, sincr, REDIS_REPLY_ARRAY);

auto ctx = r.getContext();

map<string, string> map;
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

@pavel-shirshov pavel-shirshov Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok


return map;
}

vector<string> RedisClient::keys(const string &key)
{
RedisCommand skeys;
Expand Down Expand Up @@ -94,7 +145,7 @@ shared_ptr<string> RedisClient::get(const string &key)
sget.format("GET %s", key.c_str());
RedisReply r(m_db, sget);
auto reply = r.getContext();

if (reply->type == REDIS_REPLY_NIL)
{
return shared_ptr<string>(NULL);
Expand Down
5 changes: 5 additions & 0 deletions common/redisclient.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,12 @@ class RedisClient

int64_t del(const std::string &key);

int64_t exists(const std::string &key);

int64_t hdel(const std::string &key, const std::string &field);

std::unordered_map<std::string, std::string> hgetall(const std::string &key);
std::map<std::string, std::string> hgetallordered(const std::string &key);

std::vector<std::string> keys(const std::string &key);

Expand All @@ -41,6 +44,8 @@ class RedisClient
void mset(const std::unordered_map<std::string, std::string> &map);

void hmset(const std::string &key, const std::unordered_map<std::string, std::string> &map);
void hmset(const std::string &key, const std::vector<FieldValueTuple> &values);
void hmset(const std::string &key, const std::map<std::string, std::string> &vmap);

std::shared_ptr<std::string> get(const std::string &key);

Expand Down
7 changes: 6 additions & 1 deletion common/schema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ namespace swss {
#define PFC_WD_DB 5
#define FLEX_COUNTER_DB 5
#define STATE_DB 6
#define RESTORE_DB 7
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jul 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, wrong push. Should be gone now.


/***** APPLICATION DATABASE *****/

Expand All @@ -32,6 +33,8 @@ namespace swss {

#define APP_COPP_TABLE_NAME "COPP_TABLE"

#define APP_WARM_RESTART_TABLE_NAME "WARM_RESTART_TABLE"

/***** TO BE REMOVED *****/

#define APP_TC_TO_QUEUE_MAP_TABLE_NAME "TC_TO_QUEUE_MAP_TABLE"
Expand Down Expand Up @@ -77,7 +80,7 @@ namespace swss {
#define PFC_WD_STATE_TABLE "PFC_WD_STATE_TABLE"
#define PFC_WD_PORT_COUNTER_ID_LIST "PORT_COUNTER_ID_LIST"
#define PFC_WD_QUEUE_COUNTER_ID_LIST "QUEUE_COUNTER_ID_LIST"
#define PFC_WD_QUEUE_ATTR_ID_LIST "QUEUE_ATTR_ID_LIST"
#define PFC_WD_QUEUE_ATTR_ID_LIST "QUEUE_ATTR_ID_LIST"
#define PLUGIN_TABLE "PLUGIN_TABLE"
#define LUA_PLUGIN_TYPE "LUA_PLUGIN_TYPE"
#define SAI_OBJECT_TYPE "SAI_OBJECT_TYPE"
Expand Down Expand Up @@ -141,6 +144,8 @@ namespace swss {
#define CFG_BUFFER_PORT_INGRESS_PROFILE_LIST_NAME "BUFFER_PORT_INGRESS_PROFILE_LIST"
#define CFG_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME "BUFFER_PORT_EGRESS_PROFILE_LIST"


#define CFG_WARM_RESTART_TABLE_NAME "WARM_RESTART"
/***** STATE DATABASE *****/

#define STATE_PORT_TABLE_NAME "PORT_TABLE"
Expand Down
4 changes: 3 additions & 1 deletion common/table.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ typedef std::map<std::string,TableMap> TableDump;
class TableBase {
public:
TableBase(int dbId, const std::string &tableName)
: m_tableName(tableName)
: m_tableName(tableName), m_dbId(dbId)
{
/* Look up table separator for the provided DB */
auto it = tableNameSeparatorMap.find(dbId);
Expand All @@ -52,6 +52,7 @@ class TableBase {
}

std::string getTableName() const { return m_tableName; }
int getDbId() const { return m_dbId; }

/* Return the actual key name as a combination of tableName<table_separator>key */
std::string getKeyName(const std::string &key)
Expand All @@ -74,6 +75,7 @@ class TableBase {

std::string m_tableName;
std::string m_tableSeparator;
int m_dbId;
};

class TableEntryWritable {
Expand Down