Populating m_dbName in dbConnector constructor. #589
Conversation
|
|
bocon13
force-pushed
the
dev/ntoorchi-db-name
branch
from
553a58b to
283d620
Compare
| @@ -287,6 +287,41 @@ int SonicDBConfig::getDbId(const string &dbName, const string &netns) | |||
| return getDbInfo(dbName, netns).dbId; | |||
| } | |||
|
|
|||
| string SonicDBConfig::getDbName(int dbId, const string &netns) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
|
@qiluo-msft please review |
common/dbconnector.cpp
Outdated
| @@ -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) | |||
There was a problem hiding this comment.
| 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.
common/dbconnector.cpp
Outdated
|
|
||
| SonicDBInfo& SonicDBConfig::getDbInfo(const std::string &dbName, const std::string &netns) | ||
| { | ||
| auto infos = getDbEntry(netns); |
There was a problem hiding this comment.
| auto infos = getDbEntry(netns); | |
| auto const& infos = getDbEntry(netns); |
We want to make sure that we are taking a read-only reference.
| } | ||
|
|
||
| SonicDBInfo& SonicDBConfig::getDbInfo(const std::string &dbName, const std::string &netns) | ||
| { |
There was a problem hiding this comment.
| { | |
| { | |
| 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.
| @@ -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) | |||
| { | |||
| string db_name; | |||
There was a problem hiding this comment.
Same as above.
- hold the log
- log entry into the function
- make sure we have a reference to the entry map
common/dbconnector.h
Outdated
| @@ -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); | |||
There was a problem hiding this comment.
Suggest renaming this to getDbEntryMap?
Also make sure the signature matches whatever we put in the .cc file
| @@ -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) | |||
There was a problem hiding this comment.
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.
Currently getDbName() returns back an empty string as m_dbName is not populated when constructing dbConnector based on dbId. Here SonicDBConfig::getDbName method is added to find db name based on dbId and populating m_dbName in dbConnector constructor.
Signed-off-by: Niloofar Toorchi [email protected]