Skip to content
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

Jump to bottom

Populating m_dbName in dbConnector constructor. #589

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Populating m_dbName in dbConnector constructor. #589

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
283d620
Populating m_dbName in dbConnector constructor.
ntoorchi Mar 17, 2022
df9181b
Refactoring getDbName and getDbInfo to avoid shared code.
ntoorchi May 4, 2022
a54c513
clean up.
ntoorchi May 4, 2022
cb4fc6c
holding the lock while accessing the m_db_info structure
ntoorchi May 23, 2022
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
27 changes: 25 additions & 2 deletions common/dbconnector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ void SonicDBConfig::validateNamespace(const string &netns)
}
}

SonicDBInfo& SonicDBConfig::getDbInfo(const std::string &dbName, const std::string &netns)
std::unordered_map<std::string, SonicDBInfo> SonicDBConfig::getDbEntry(const std::string &netns)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
std::unordered_map<std::string, SonicDBInfo> SonicDBConfig::getDbEntry(const std::string &netns)
const std::unordered_map<std::string, SonicDBInfo>& SonicDBConfig::getDbEntry(const std::string &netns)

We need to avoid copying the map.

{
std::lock_guard<std::recursive_mutex> guard(m_db_info_mutex);

Expand All @@ -232,7 +232,12 @@ SonicDBInfo& SonicDBConfig::getDbInfo(const std::string &dbName, const std::stri
SWSS_LOG_ERROR("%s", msg.c_str());
throw out_of_range(msg);
}
auto& infos = foundNetns->second;
return foundNetns->second;
}

SonicDBInfo& SonicDBConfig::getDbInfo(const std::string &dbName, const std::string &netns)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{
{
std::lock_guard<std::recursive_mutex> guard(m_db_info_mutex);
SWSS_LOG_ENTER();

Make sure to hold the lock while we are accessing the m_db_info structure. Also, the convention is to log entry into the method.

auto infos = getDbEntry(netns);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
auto infos = getDbEntry(netns);
auto const& infos = getDbEntry(netns);

We want to make sure that we are taking a read-only reference.

auto foundDb = infos.find(dbName);
if (foundDb == infos.end())
{
Expand Down Expand Up @@ -287,6 +292,20 @@ int SonicDBConfig::getDbId(const string &dbName, const string &netns)
return getDbInfo(dbName, netns).dbId;
}

string SonicDBConfig::getDbName(int dbId, const string &netns)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a mostly copy-paste of SonicDBConfig::getSeparator. Can we refactor the two methods to call a common getDbInfo method that contains the common code?

Copy link
Author

Choose a reason for hiding this comment

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

We can extract the common part from getDbInfo and getDbName and call it as another method (getDbEntry). getSeparator, however, is using m_db_separator instead of m_db_info. m_db_info also has the information on separator, so it can use the same procedure as getDbName to find the separator based on dbId and then there would be no need to have duplicated information stored in m_db_separator.
@prsunny @qiluo-msft can you please review this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

getDbName

It is possible that one dbId is mapped to multiple dbName. Like FLEX_COUNTER_DB and PFC_WD_DB are all 5.

Why we need this function?

Copy link
Author

Choose a reason for hiding this comment

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

getDbName(dbId) is needed to populate db_name when creating a DBConnector using a dbId (DBConnector::DBConnector(int dbId, const RedisContext& ctx)), which we call it in pins-infra at p4rt_app/p4rt.cc. Otherwise, DBConnector-> getDbName() returns an empty string. We might need to get the db name for logging purposes.
The majority of dbs have unique ID (as it is expected). Not sure why FLEX_COUNTER_DB and PFC_WD_DB have the same id, but I think their DBConnector can be created through dbName instead of dbId, and then there is no issue.

{
string db_name;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

  1. hold the log
  2. log entry into the function
  3. make sure we have a reference to the entry map

auto infos = getDbEntry(netns);
for ( auto it = infos.begin(); it != infos.end(); ++it ) {
auto& db_info = it->second;
if (db_info.dbId == dbId) {
db_name = it->first;
break;
}
}
return db_name;
}

string SonicDBConfig::getSeparator(const string &dbName, const string &netns)
{
return getDbInfo(dbName, netns).separator;
Expand Down Expand Up @@ -510,6 +529,7 @@ void DBConnector::select(DBConnector *db)
DBConnector::DBConnector(const DBConnector &other)
: RedisContext(other)
, m_dbId(other.m_dbId)
, m_dbName(other.m_dbName)
ntoorchi marked this conversation as resolved.
Show resolved Hide resolved
, m_namespace(other.m_namespace)
{
select(this);
Expand All @@ -518,6 +538,7 @@ DBConnector::DBConnector(const DBConnector &other)
DBConnector::DBConnector(int dbId, const RedisContext& ctx)
: RedisContext(ctx)
, m_dbId(dbId)
, m_dbName(SonicDBConfig::getDbName(dbId))
, m_namespace(EMPTY_NAMESPACE)
{
select(this);
Expand All @@ -526,6 +547,7 @@ DBConnector::DBConnector(int dbId, const RedisContext& ctx)
DBConnector::DBConnector(int dbId, const string& hostname, int port,
unsigned int timeout)
: m_dbId(dbId)
, m_dbName(SonicDBConfig::getDbName(dbId))
, m_namespace(EMPTY_NAMESPACE)
{
struct timeval tv = {0, (suseconds_t)timeout * 1000};
Expand All @@ -537,6 +559,7 @@ DBConnector::DBConnector(int dbId, const string& hostname, int port,

DBConnector::DBConnector(int dbId, const string& unixPath, unsigned int timeout)
: m_dbId(dbId)
, m_dbName(SonicDBConfig::getDbName(dbId))
, m_namespace(EMPTY_NAMESPACE)
{
struct timeval tv = {0, (suseconds_t)timeout * 1000};
Expand Down
2 changes: 2 additions & 0 deletions common/dbconnector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ class SonicDBConfig
static void validateNamespace(const std::string &netns);
static std::string getDbInst(const std::string &dbName, const std::string &netns = EMPTY_NAMESPACE);
static int getDbId(const std::string &dbName, const std::string &netns = EMPTY_NAMESPACE);
static std::string getDbName(int dbId, const std::string &netns = EMPTY_NAMESPACE);
static std::string getSeparator(const std::string &dbName, const std::string &netns = EMPTY_NAMESPACE);
static std::string getSeparator(int dbId, const std::string &netns = EMPTY_NAMESPACE);
static std::string getSeparator(const DBConnector* db);
Expand Down Expand Up @@ -96,6 +97,7 @@ class SonicDBConfig
std::unordered_map<std::string, RedisInstInfo> &inst_entry,
std::unordered_map<std::string, SonicDBInfo> &db_entry,
std::unordered_map<int, std::string> &separator_entry);
static std::unordered_map<std::string, SonicDBInfo> getDbEntry(const std::string &netns = EMPTY_NAMESPACE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest renaming this to getDbEntryMap?

Also make sure the signature matches whatever we put in the .cc file

static SonicDBInfo& getDbInfo(const std::string &dbName, const std::string &netns = EMPTY_NAMESPACE);
static RedisInstInfo& getRedisInfo(const std::string &dbName, const std::string &netns = EMPTY_NAMESPACE);
};
Expand Down