Skip to content

[POC] POC code for show default value with yang model#595

Closed
liuh-80 wants to merge 24 commits intosonic-net:masterfrom
liuh-80:dev/liuh/swss_default_value
Closed

[POC] POC code for show default value with yang model#595
liuh-80 wants to merge 24 commits intosonic-net:masterfrom
liuh-80:dev/liuh/swss_default_value

Conversation

@liuh-80
Copy link
Copy Markdown
Contributor

@liuh-80 liuh-80 commented Apr 1, 2022

No description provided.

@liuh-80 liuh-80 force-pushed the dev/liuh/swss_default_value branch from 5f97c92 to 69a2af9 Compare April 15, 2022 05:59
return shared_ptr<string>(NULL);
}

size_t pos = key.find("|");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"|"

The separator ground truth is from SonicDBConfig. Do not hard code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed, now get separator from DBConnector.

return hget(key, field, false);
}

shared_ptr<string> DBConnector::hget(const string &key, const string &field, bool withDefaultValue)
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Apr 23, 2022

Choose a reason for hiding this comment

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

DBConnector

From the design point of view, DBConnector is in the Redis concept domain and it has no knowledge of ConfigDB, table, row and default value. So it has no requirement to engaged into this new feature.

The same applies to DBInterface, SonicV2Connector, Logger, RedisClient.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed by create a ConfigDBDecorator class and move this code to this class.

[[deprecated("Please use get_entry(std::string table, std::string key, bool withDefaultValue) instead.")]]
#endif
std::map<std::string, std::string> get_entry(std::string table, std::string key);
std::map<std::string, std::string> get_entry(std::string table, std::string key, bool withDefaultValue);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

withDefaultValue

I see you keeps backward-compatible to help gradually migration. I support the idea.

Instead of customize per function call, I think customize the constructor will be good enough. So the migration is instance by instance.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will update this PR and design document according to your comments and discussion.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed, remove all new parameter and revert UT change.

@lguohan
Copy link
Copy Markdown
Contributor

lguohan commented Apr 23, 2022

this is one of key feature. do we have a design doc to review?

@liuh-80
Copy link
Copy Markdown
Contributor Author

liuh-80 commented Apr 26, 2022

this is one of key feature. do we have a design doc to review?

Here is design document PR: https://github.com/Azure/SONiC/pull/989/files

@liuh-80
Copy link
Copy Markdown
Contributor Author

liuh-80 commented Apr 27, 2022

Build failed because we need libyang, however sonic version of patched libyang is in sonic-buildimage repo.
Will check if can install original libyang in pipeline and pass build.

if (reply->type == REDIS_REPLY_NIL)
{
return shared_ptr<string>(NULL);
auto dbdecortor = this->getDBDecorator();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reason does not create ConfigDBConnector but add code here is because DBinterface has following data member:

std::unordered_map<std::string, DBConnector> m_redisClient;

So, create a new derived class ConfigDBConnector need code change in this member and related code.

Will discussion if this is acceptable.

@liuh-80
Copy link
Copy Markdown
Contributor Author

liuh-80 commented Jun 16, 2022

Close this PR, POC will move to: #625

@liuh-80 liuh-80 closed this Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants