-
Notifications
You must be signed in to change notification settings - Fork 86
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.
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
[multi-DB] Part 3: Python API changes #52
Conversation
src/swsssdk/dbconnector.py
Outdated
@@ -17,23 +18,29 @@ def __init__(self, **kwargs): | |||
|
|||
SonicV1Connector.db_map = _connector_map[SonicV1Connector.__name__]['db_map'] | |||
|
|||
if len(SonicV1Connector.db_map) != len({v['db'] for k, v in SonicV1Connector.db_map.items()}): | |||
raise RuntimeError("Duplicate DB index detected in configuration.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this block? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this block?
I added the missing LOGLEVEL_DB(3) , PFC_WD_DB(5), FLEX_COUNTERS_DB(5) into existing database.json which are mainly used for python today. Since both PFC_WD_DB and FLEX_COUNTER_EB has value 5. This check will raise error. But these check is unnecessary, internally we are using string "DBNAME" as key to index(in SonicV2Connector/DBInterface), duplicated value index is fine,
"PFC_WD_DB": {
"db": 5,
"separator": ":"
},
"FLEX_COUNTER_DB": {
"db": 5,
"separator": ":"
}, #Closed
src/swsssdk/dbconnector.py
Outdated
class SonicV2Connector(DBInterface): | ||
def __init__(self, **kwargs): | ||
super(SonicV2Connector, self).__init__(**kwargs) | ||
|
||
# after py test cases(e.g snmpagent/tests) are modified | ||
# we can do the replacement on this new API via remnaming connect_new to connect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remnaming [](start = 52, length = 9)
typo #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix typo #Resolved
pass | ||
|
||
|
||
SonicV2Connector.db_map = _connector_map[SonicV2Connector.__name__]['db_map'] | ||
|
||
if len(SonicV2Connector.db_map) != len({v['db'] for k, v in SonicV2Connector.db_map.items()}): | ||
raise RuntimeError("Duplicate DB index detected in configuration.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove?
I added the missing LOGLEVEL_DB(3) , PFC_WD_DB(5), FLEX_COUNTERS_DB(5) into existing database.json which are mainly used for python today. Since both PFC_WD_DB and FLEX_COUNTER_EB has value 5. This check will raise error. But these check is unnecessary, internally we are using string "DBNAME" as key to index(in SonicV2Connector/DBInterface), duplicated value index is fine, #Closed
src/swsssdk/dbconnector.py
Outdated
# after py test cases(e.g snmpagent/tests) are modified | ||
# we can do the replacement on this new API via renaming connect_new to connect | ||
# def connect(self, db_name, retry_on=True, isTcpconn=False): | ||
def connect_new(self, db_name, retry_on=True, isTcpconn=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
connect_new [](start = 8, length = 11)
Is it possible to keep original function and change its implementation? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original connect() is from DBInterface class, which is not modified. Here I just overwrite the connect() in SonicV2Connector which is the child class to prepare some parameters, finally it is still call the original connect in DBInterface. now it is named as connect_new for temporally, since we didn't fix the py test cases yet which will cause test failed. Finally we will use name connect() which is the original name. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use connect() instead #Resolved
src/swsssdk/__init__.py
Outdated
global _sonic_db_config_init | ||
if _sonic_db_config_init == False: | ||
_load_sonic_db_config() | ||
return _sonic_db_config["DATABASES"][db_name]["id"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract as a new class #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@qiluo-msft so in the codes , I made some changes, if /var/run/redis/sonic-db/database_config.json is not there, we use the config at /usr/local/lib/python2.7/swsssdk/config/database_config.josn. This case only happened for py test cases, it is safe. |
BTW VS test passed as well. #Resolved |
The "db" fields are duplication with "id" fields in database_config.json. Some suggestions:
Refers to: src/swsssdk/config/database.json:54 in a6aab64. [](commit_id = a6aab64, deletion_comment = False) |
_sonic_db_config = {} | ||
|
||
@staticmethod | ||
def load_sonic_db_config(sonic_db_file_path=SONIC_DB_CONFIG_FILE): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
load_sonic_db_config [](start = 8, length = 20)
I guess you could rewrite _load_connector_map instead of creating a new function.
Should we remove SonicV2Connector.db_map
and _load_connector_map
at all? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/swsssdk/dbconnector.py
Outdated
return SonicDBConfig._sonic_db_config["INSTANCES"][inst_name]["port"] | ||
|
||
@staticmethod | ||
def get_sonic_db_id(db_name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_sonic_db_id [](start = 8, length = 15)
I guess you could override get_dbid instead of creating a new function.
Should we remove the member function get_dbid
?
#Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
if len(SonicV1Connector.db_map) != len({v['db'] for k, v in SonicV1Connector.db_map.items()}): | ||
raise RuntimeError("Duplicate DB index detected in configuration.") | ||
|
||
|
||
class SonicV2Connector(DBInterface): | ||
def __init__(self, **kwargs): | ||
super(SonicV2Connector, self).__init__(**kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super(SonicV2Connector, self).init(**kwargs) [](start = 8, length = 48)
It makes no sense to reuse parent class __init__
. Suggest add one option is_unix_socket
for each object with default value true, and remove isTcpconn
from connect
#Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. invoking parent init is necessary , since it initialized some variable/struct
Even with the difficulty in python wheel, we should try workaround to achieve the goal we discussed. Please help explorer. At least I know In reply to: 538191085 [](ancestors = 538191085) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented
a6aab64
to
0f05483
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- redesign class DBinterface , SonivV2Connector and SonicDBConfig
2 DBinterface is the raw interface to connect a DB instance and accept the parameter as what we have today(hostname, port, socket) , the big changes is DBInterface APIs no longer consider DB name as parameter, it only accept raw dbid as parameter.
3 SonicDBConfig class handle all the db configuration related data. It store the mapping between dbname and dbid as well as the isntance info and socket/hostname/port onformation. SonicDBConfig also provide get API for others to access these data.
4 SonicV2Config class behaves as a bridge between SonicDBConfig and DBinterface. Its parent is DBinterface. Application use SonicV2Connector to connect to a db and write/read data from db. In SonicV2Connector, we do translation between dbname and dbid, also it provides the same signature as what the application is using today. Then we don't need to change the application codes.
Here having some root access issue as we discussed offline, I suggest keep the library database_config.json file in the library domain , this won't affect anything on pyhsical machine. It helps solve the testing issue. For swss-common library, I suggest the same, with library there is a database_config.json file for testing only. Later when all the stuff are done, we can focus on how to solve the root access issue during building images and how to distribute the file to a single place. For now , to me it is OK, it doesn't affect any functionality .
|
DONE |
@qiluo-msft redesign the codes, the changes is a little bit more, we can have a chat to explain what the idea is and how it works if possible. |
src/swsssdk/dbconnector.py
Outdated
super(SonicV2Connector, self).__init__(**kwargs) | ||
self.use_unix_socket_path = unix_socket_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unix_socket_path [](start = 36, length = 16)
Suggestion: unix_socket_path->use_unix_socket_path #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DONE
src/swsssdk/dbconnector.py
Outdated
return SonicDBConfig._sonic_db_config["DATABASES"].keys() | ||
|
||
@staticmethod | ||
def get_sonic_db_socket(db_name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_sonic_db_socket [](start = 8, length = 19)
Suggest rename: get_sonic_db_* -> get_* #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DONE
def get_sonic_db_socket(db_name): | ||
if SonicDBConfig._sonic_db_config_init == False: | ||
SonicDBConfig.load_sonic_db_config() | ||
inst_name = SonicDBConfig._sonic_db_config["DATABASES"][db_name]["instance"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
db_name [](start = 64, length = 7)
Should we handle index out of range explicitly?
- if db_name does not exist, it's invalid argument
- if inst_name does not exist, it's invalid config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added db_name_validation() and inst_name_validation(), if invalid , raise a error
def get_sonic_db_socket(db_name): | ||
if SonicDBConfig._sonic_db_config_init == False: | ||
SonicDBConfig.load_sonic_db_config() | ||
inst_name = SonicDBConfig._sonic_db_config["DATABASES"][db_name]["instance"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract a common helper function like get_instance? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DONE
|
||
try: | ||
if os.path.isfile(sonic_db_file_path) == False: | ||
sonic_db_file_path = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'config', 'database_config.json') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sonic_db_file_path = os.path.join(os.path.dirname(os.path.abspath(file)), 'config', 'database_config.json') [](start = 16, length = 111)
We don't want to see this case in production. Can we add a syslog warning? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added warning log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Minor issues remain.
updated |
* multiDB part3 : python API * add validation check and update some minor suggestions
add database_config.json parsing logic
add get_hostname/port/sokcet/dbid APIs
make existing database.json contains all DB define, replace old database.json
remove unnecessary check in dbconnector.py to count duplicated dbid values
add SonicV2Connector connect() API to have the ability to choose db instances(host,port or socket) , then setting argvs and call DBInterface connector as what we did today.
add a load interface to allow test cases to load config file.
redesign DBInterface, SonicV2Connector
DBinterface is the raw interface to connect a DB instance and accept the parameter as what we have today(hostname, port, socket) , the big changes is DBInterface APIs no longer consider DB name as parameter, it only accept raw dbid as parameter.
SonicDBConfig class handle all the db configuration related data. It store the mapping between dbname and dbid as well as the isntance info and socket/hostname/port onformation. SonicDBConfig also provide get API for others to access these data.
SonicV2Config class behaves as a bridge between SonicDBConfig and DBinterface. Its parent is DBinterface. Application use SonicV2Connector to connect to a db and write/read data from db. In SonicV2Connector, we do translation between dbname and dbid, also it provides the same signature as what the application is using today. Then we don't need to change the application codes.
Signed-off-by: Dong Zhang [email protected]