-
Notifications
You must be signed in to change notification settings - Fork 322
Relocate sonicdb and sonicdb-derive to swss-common #1077
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
base: master
Are you sure you want to change the base?
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
zjswhhh
left a comment
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.
lgm
| use swss_common::KeyOpFieldValues; | ||
|
|
||
| /// Trait for objects that can be stored in a Sonic DB table. | ||
| pub trait SonicDbTable { |
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.
SonicDbTable seems a combination of SonicDBInfo and Table. Is this really necessary?
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.
It is for a totally different purpose and also provides different info. The primary usage is to be used along with sonicdb-derive, which is to annotate structs for sonicdb tables. Here is one usage https://github.com/sonic-net/sonic-dash-ha/blob/d14d54bb1f1f81307e8ddd76905bbf57a9546198/crates/hamgrd/src/db_structs.rs#L219. It allows designer adding table meta data, including table name, db name, protobuf-encoded etc to a struct. Otherwise, we have to maintain the mapping from struct-type using separate data structure, which is not good for maintenance and readability.
SonicDbInfo provides DB level info and Table is for table access which is nothing SonicDbTable about.
In summary, sonicdb and sonicdb-derive allow adding table meta data to a struct, which SonicDBInfo and Table cannot do.
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.
@qiluo-msft please let me know if you have further questions. thanks
|
Hi @qiluo-msft - can you help review again please? |
|
Offline discussed, @yue-fred-gao will explore relocate them to sonic-dash-api repo. |
why
currently, sonic-dash-ha adds sonic-dash-api as submodule to compile protos into rust structs. At the same time, both are submodules in sonic-buildimage. This creates unnecessary dependency and complexity. The solution is moving sonic-dash-api-proto crate into sonic-dash-api. sonic-dash-ha adds sonic-dash-api-proto as a dependent crate. At build time, it pulls the crate through git and the latter compiles protos on demand. This is similar to swss-common crate to sonic-dash-ha. However, sonic-dash-api-proto depends on sonicdb-derive and sonic-common crates in sonic-dash-ha. To break the inter-dependency, we need to move sonicdb-derive and the SonicDBTable in sonic-common, which is added to a new crate called sonicdb to another place. swss-common is the most natural location because it is a derive macro for SONiC db and it potentially can be used by other components in sonic.
what this PR does
Move sonicdb-derive from sonic-dash-ha to swss-common.
Move SonicDBTable that was previously in sonic-common crate in sonic-dash-ha to the sonicdb crate in swss-common.
NOTE: rust currently requires derive macro to be in its own crate so we have to use 2 crates.
dependent PRs
sonic-net/sonic-dash-api#45
sonic-net/sonic-dash-ha#109