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

Jump to bottom

[dbconnect]: Support DPU database schema #845

Merged
merged 11 commits into from
Jan 27, 2024
Merged

[dbconnect]: Support DPU database schema #845

merged 11 commits into from
Jan 27, 2024

Conversation

Pterosaur
Copy link
Contributor

@Pterosaur Pterosaur commented Dec 28, 2023 •
edited
Loading

This PR is for expanding the capability of dbconnect to adapt the new DPU DB schema introduced by PR: sonic-net/sonic-buildimage#17443

@Pterosaur Pterosaur changed the title support DPU database schema [dbconnect]: Support DPU database schema Dec 29, 2023
@Pterosaur
Add unittest
f4fa7ce 
Signed-off-by: Ze Gan <[email protected]>
@Pterosaur Pterosaur marked this pull request as ready for review December 29, 2023 12:36
qiluo-msft
qiluo-msft reviewed Jan 11, 2024
View reviewed changes
common/dbconnector.h
return containerName.empty() && netns.empty();
}

std::string toString() const
Copy link
Contributor

@qiluo-msft qiluo-msft Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

toString

If the name is flexible, use to_string instead? #WontFix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it is a tradeoff, align to the STL convertion or align to this file's convertion .
So, I choose to align to this file.

qiluo-msft
qiluo-msft reviewed Jan 11, 2024
View reviewed changes
common/dbconnector.h
{
buffer.push_back(netns);
}
return boost::algorithm::join(buffer, ":");
Copy link
Contributor

@qiluo-msft qiluo-msft Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

join

If one has only containerName, and another has only netns (the same string), they will have the same toString. Is it going to be problematic? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's a problem. Fixed it.

qiluo-msft
qiluo-msft reviewed Jan 11, 2024
View reviewed changes
common/dbconnector.h
@@ -62,14 +110,21 @@ class SonicDBConfig
static void reset();

static void validateNamespace(const std::string &netns);
static std::string getDbInst(const std::string &dbName, const std::string &netns = EMPTY_NAMESPACE);
Copy link
Contributor

@qiluo-msft qiluo-msft Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

netns

Is it easier just to add one more parameter containerName=DEFAULT_CONTAINERNAME to each function related ? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't feel that adding more parameters is better. Because I have introduced a new class SonicDBKey, I believe we should prefer to use this new class's object to identify a database instance instead of the old style API in the following code. This function is just for adapting the existing reference.

Copy link
Contributor

Choose a reason for hiding this comment

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

The suggested function is like

    static std::string getDbInst(const std::string &dbName, const std::string &netns = EMPTY_NAMESPACE, containerName=DEFAULT_CONTAINERNAME);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

qiluo-msft
qiluo-msft reviewed Jan 11, 2024
View reviewed changes
common/dbconnector.h
@@ -80,27 +135,29 @@ class SonicDBConfig
%}
#endif

static std::vector<std::string> getDbList(const std::string &netns = EMPTY_NAMESPACE);
Copy link
Contributor

@qiluo-msft qiluo-msft Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

getDbList

the same: add one more parameter containerName=DEFAULT_CONTAINERNAME #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reply as above.

qiluo-msft
qiluo-msft reviewed Jan 11, 2024
View reviewed changes
common/dbconnector.h
@@ -158,6 +215,7 @@ class DBConnector : public RedisContext
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 &netns);
Copy link
Contributor

@qiluo-msft qiluo-msft Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

netns

the same: add one more parameter containerName=DEFAULT_CONTAINERNAME #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reply as above.

@Pterosaur
Rename schema field
4fe98b0 
Signed-off-by: Ze Gan <[email protected]>
@Pterosaur
Fix ut
57d09d3 
Signed-off-by: Ze Gan <[email protected]>
@Pterosaur Pterosaur requested a review from qiluo-msft January 15, 2024 10:08
r12f
r12f reviewed Jan 18, 2024
View reviewed changes
common/dbconnector.cpp Outdated
string socket;
if (it.value().find("unix_socket_path") != it.value().end())
{
socket = it.value().at("unix_socket_path");
Copy link
Contributor

Choose a reason for hiding this comment

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

we should be able to directly get the value from the iterator that returned by find(), according to nlohmann/json code here: https://github.com/nlohmann/json/blob/a259ecc51e1951e12f757ce17db958e9881e9c6c/include/nlohmann/detail/iterators/iter_impl.hpp#L280C21-L280C21.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

common/dbconnector.cpp Outdated
m_db_info[EMPTY_NAMESPACE] = db_entry;
m_db_separator[EMPTY_NAMESPACE] = separator_entry;
m_inst_info[empty_key] = inst_entry;
m_db_info[empty_key] = db_entry;
Copy link
Contributor

Choose a reason for hiding this comment

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

use .emplace(empty_key, std::move(db_entry)) will be faster if it is supported in our dev env.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

common/dbconnector.cpp Outdated

if(element["namespace"].empty())
if (!element["database_name"].empty())
Copy link
Contributor

Choose a reason for hiding this comment

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

have we decided the name to be database_name? I remember Qi had concern on this name, since redis also has a database concepts, but not sure if this is closed or not.

if not, maybe database_instance_name or something like this might help with the concern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been changed to container_name, please check it again.

@Pterosaur
Fix comment
bd95490 
Signed-off-by: Ze Gan <[email protected]>
@Pterosaur Pterosaur requested a review from r12f January 18, 2024 04:46
@Pterosaur
Copy link
Contributor Author

Hi @qiluo-msft Please help to review or approve/merge this PR?

@Pterosaur
Fix comment
bb4f1df 
Signed-off-by: Ze Gan <[email protected]>
qiluo-msft
qiluo-msft reviewed Jan 25, 2024
View reviewed changes
common/dbconnector.cpp
{
struct timeval tv = {0, (suseconds_t)timeout * 1000};
struct timeval *ptv = timeout ? &tv : NULL;
if (isTcpConn)
{
initContext(SonicDBConfig::getDbHostname(dbName, netns).c_str(), SonicDBConfig::getDbPort(dbName, netns), ptv);
initContext(SonicDBConfig::getDbHostname(dbName, m_key).c_str(), SonicDBConfig::getDbPort(dbName, m_key), ptv);
Copy link
Contributor

Choose a reason for hiding this comment

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

SonicDBConfig

Future TODO: decouple DBConnector with SonicDBConfig. DBConnector only need to know its own dbName/index, port, hostname, unix socket. No need to know any other information such as namespace, containerName, sep, etc.

@qiluo-msft qiluo-msft mentioned this pull request Jan 25, 2024
Open
@qiluo-msft qiluo-msft merged commit 41ee154 into sonic-net:master Jan 27, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r12f r12f r12f approved these changes

@qiluo-msft qiluo-msft qiluo-msft approved these changes

@liuh-80 liuh-80 Awaiting requested review from liuh-80

@ganglyu ganglyu Awaiting requested review from ganglyu

@judyjoseph judyjoseph Awaiting requested review from judyjoseph

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Pterosaur @r12f @qiluo-msft