Update SonicDBConfig calls for centralize_database to use C++ APIs#1441
Update SonicDBConfig calls for centralize_database to use C++ APIs#1441vaibhavhd merged 1 commit intosonic-net:masterfrom
Conversation
|
Retest this please |
| def centralize_to_target_db(target_dbname): | ||
| target_dbport = SonicDBConfig.get_port(target_dbname) | ||
| target_dbhost = SonicDBConfig.get_hostname(target_dbname) | ||
| target_dbport = SonicDBConfig.getDbPort(target_dbname) |
There was a problem hiding this comment.
getDbPort [](start = 34, length = 9)
Thanks for the fixes! They are valid. There is another way to use swig to translate the names by a single wrapper like https://github.com/Azure/sonic-swss-common/blob/9e91e0d891398b468b8087682ae91335791fac51/common/dbconnector.h#L76.
If the functions are used in multiple repos, the renaming will be better.
There was a problem hiding this comment.
I see, thanks for the info!
I was thinking that we are planning to deprecate the Python APIs, and want to stick to C++ library.
If that is correct understanding, then shouldn't we move away from using SWIG wrappers?
If the functions are used in multiple repos, the renaming will be better.
In this case, we should have similar change as in this PR for other repos too. Basically, to maintain C++ method names, and less of translated Python methods.
Thoughts?
There was a problem hiding this comment.
It's a trade-off: Adding SWIG wrappers means more code to maintain, but allows us to align the naming convention with PEP8, where function/method names should be lowercase_with_underscores, whereas in C++ our function names are camel case.
|
@qiluo-msft, the failure seen in the tests seems to be unrealted, and has failed all the recent builds in https://sonic-jenkins.westus2.cloudapp.azure.com/job/common/job/sonic-utilities-build-pr I have started a separate email thread for the failure. Can we go ahead and merge this change? |
|
We need to wait the build fix first, and merge this one after all build checkers pass. |
|
Retest this please |
1 similar comment
|
Retest this please |
The DB backup during warmboot has started failing recently after the changes made to deprecate the usage of SonicDBConfig methods originally implemented by python in https://github.com/Azure/sonic-py-swsssdk/. The new implementation is based on hiredis C++ library. How I did it: The centralize_database script still uses the Python APIs instead of C++, update the method names which are now defined in sonic-swss-common. With the new changes, the warm-boot goes ahead without DB save errors.
3 participants
What I did
Fixes: sonic-net/sonic-buildimage#6811
The DB backup during warmboot has started failing recently after the changes made in commit (2e1f354#diff-5777288143b5a8ff1e97be074d748fd825f0ee1812f2d7c65d850c893bccefd6)
Above change was an attempt to deprecate the
SonicDBConfigmethods originally implemented by python in https://github.com/Azure/sonic-py-swsssdk/. The new implementation is based on hiredis C++ library.How I did it
The
centralize_databasescript still uses the Python APIs instead of C++, update the method names which are now defined insonic-swss-commonhttps://github.com/Azure/sonic-swss-common/blob/master/common/dbconnector.h#L64How to verify it
With the changes, the warmboot now works without an error:
Also, the
test_warm_rebootnow passes on the devices:Previous command output (if the output of a command-line utility has changed)
New command output (if the output of a command-line utility has changed)