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

Update SonicDBConfig calls for centralize_database to use C++ APIs #1441

Merged
merged 1 commit into from
Feb 22, 2021
Merged

Update SonicDBConfig calls for centralize_database to use C++ APIs #1441

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 8 additions & 8 deletions scripts/centralize_database
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,19 @@ import redis
import argparse

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@vaibhavhd vaibhavhd Feb 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

target_dbhost = SonicDBConfig.getDbHostname(target_dbname)

dblists = SonicDBConfig.get_dblist()
dblists = SonicDBConfig.getDbList()
for dbname in dblists:
dbport = SonicDBConfig.get_port(dbname)
dbhost = SonicDBConfig.get_hostname(dbname)
dbport = SonicDBConfig.getDbPort(dbname)
dbhost = SonicDBConfig.getDbHostname(dbname)
# if the db is on the same instance, no need to move
if dbport == target_dbport and dbhost == target_dbhost:
continue

dbsocket = SonicDBConfig.get_socket(dbname)
dbid = SonicDBConfig.get_dbid(dbname)
dbsocket = SonicDBConfig.getDbSock(dbname)
dbid = SonicDBConfig.getDbId(dbname)

r = redis.Redis(host=dbhost, unix_socket_path=dbsocket, db=dbid)

Expand Down Expand Up @@ -49,7 +49,7 @@ Example : centralize_database APPL_DB
if args.target_db:
try:
centralize_to_target_db(args.target_db)
print(SonicDBConfig.get_instancename(args.target_db))
print(SonicDBConfig.getDbInst(args.target_db))
except Exception as ex:
template = "An exception of type {0} occurred. Arguments:\n{1!r}"
message = template.format(type(ex).__name__, ex.args)
Expand Down