Reintroducing PR #364 - DBconnector to support namespaces. #371
Conversation
…-swss#1363 Also changes to allign the dbConnector class here with that of sonic-py-swssdk 1. Ignore if the database_global.json file is not present. 2. validate the namespace before using it.
|
retest this please |
common/dbconnector.cpp
Outdated
| m_global_init = true; | ||
|
|
||
| // Make regular init also done | ||
| m_init = true; |
There was a problem hiding this comment.
Please make the logic same as python version #Closed
There was a problem hiding this comment.
Sure, @qiluo-msft I was thinking about this yesterday, I finally moved it here as I thought this is logical to set the init flags to TRUE only if the database_global.json file exists and we have parsed it correct ?
Let me know what you think , I could change it back in python version if you too feel the same.
There was a problem hiding this comment.
If there is a bug in python version, we need to fix it first.
If not, make the C++ version behaves the same as python version.
In reply to: 464586440 [](ancestors = 464586440)
There was a problem hiding this comment.
I have updated so that it matches the python version. I agree that setting the flag m_global_init is ok here -- because this is an API which is invoked by user and he should provide the valid database_global.json file as input.
common/dbconnector.cpp
Outdated
| } | ||
| } | ||
|
|
||
| void SonicDBConfig::initialize(const string &file, const string &nameSpace) |
There was a problem hiding this comment.
const string &nameSpace [](start = 51, length = 23)
Why this function has extra parameter than the python version load_sonic_db_config ? They should be the same. #Closed
There was a problem hiding this comment.
Will update - thanks !
|
Is this comment on previous PR is addressed: a) why it has to be different ? It has be consistent . Any particular reason why it need to be different ? Application should be doing same irrespective of library being used. b)Also it does not work if we don't call SonicDBConfig::initializeGlobalConfig. |
Yes @abdosi we don't fail now if we the database_global.json is not present |
| getContext()->unix_sock.path, | ||
| timeout); | ||
| ret->m_dbName = m_dbName; | ||
| ret = new DBConnector(getDbName(), timeout, (getContext()->connection_type == REDIS_CONN_TCP), getNamespace()); |
There was a problem hiding this comment.
There was a failure I was debugging in sonic-swss/tests/mock_tests, even with the change in the PR where we do constructor delegation.
The reason being in this line 418, when we create a newDBConnector we call the DBConnector API with namespace argument. Since we don't define DBConnector() constructor with the namespace argument in the sonic-swss/tests/mock_tests/mock_dbconnector.cpp .. it come to this actual non-mocked handler and fails .
make[3]: Entering directory '/sonic/src/sonic-swss/tests/mock_tests'
make[4]: Entering directory '/sonic/src/sonic-swss/tests/mock_tests'
../../config/test-driver: line 107: 12950 Aborted (core dumped) "$@" > $log_file 2>&1
FAIL: tests
============================================================================
Testsuite summary for sonic-swss 1.0
============================================================================
# TOTAL: 1
# PASS: 0
# SKIP: 0
# XFAIL: 0
# FAIL: 1
# XPASS: 0
# ERROR: 0
terminate called after throwing an instance of 'std::system_error'
what(): Unable to connect to redis: Cannot assign requested address
FAIL tests (exit status: 134)
Two options to fix this
-
Add the fix Update DBConnector definition in mock_dbconnector.cpp sonic-swss#1362. Though there are tests failing in swss, they are not related.
-
Other option is to revert this function DBConnector::newConnector(unsigned int timeout) , to as it was before (https://github.com/Azure/sonic-swss-common/blob/00bcd1d0acfdb1fd33362beafaf41ea3f5c301aa/common/dbconnector.cpp#L243). So this API won't support the usage of namespace for now , can add it back later.
|
can you update submodule ? is it still blocked on sonic-swss ? |
Yes there is still a VS PR build failure .. though locally build went fine with the changes. To look at why the PR build fails |
)" This reverts commit 1943e2b.
| @@ -60,12 +79,14 @@ class DBConnector | |||
| DBConnector(int dbId, const std::string &hostname, int port, unsigned int timeout); | |||
| DBConnector(int dbId, const std::string &unixPath, unsigned int timeout); | |||
| DBConnector(const std::string &dbName, unsigned int timeout, bool isTcpConn = false); | |||
| DBConnector(const std::string &dbName, unsigned int timeout, bool isTcpConn, const std::string &nameSpace); | |||
There was a problem hiding this comment.
nameSpace [](start = 100, length = 9)
nameSpace -> namespace
There was a problem hiding this comment.
@Qi cannot use namespace as this is a C++ keyword. may be namespaceName or name_space ?
There was a problem hiding this comment.
Thanks! how about netns ?
Adding PR #364 back.
It was reverted earlier as there was a build failure with sonic-swss/tests/mock_tests/mock_dbconnector.cpp failing to build due to DBConnnector constructor being redefined there (Issue mentioned in sonic-net/sonic-swss#1363).
I have addressed it now by keeping the old constructor and delegating that to the new constructor which has "namespace" as additional argument.
This PR, I have included the changes to further align the dbConnector class here in sonic-swss-common with one present in sonic-py-swssdk
1. Ignore ( don't throw exception ) if the database_global.json file is not present.
2. validate the namespace before using it.