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.

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

Use swsscommon API to read database configuration #176

Merged
merged 10 commits into from
Dec 19, 2023

Conversation

ganglyu
Copy link
Contributor

@ganglyu ganglyu commented Dec 14, 2023

Why I did it

sonic_db_config/db_config.go reads from database configuration json to get database configuration, but swsscommon has provided the same feature.

How I did it

Use swsscommon API to read database configuration.
Pending on below PR to clear database configuration for unit test.
sonic-net/sonic-swss-common#843

How to verify it

Run unit test.

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

}
return separator.(string)
separator := swsscommon.SonicDBConfigGetSeparator(db_name, ns)
return separator
}

func GetDbId(db_name string, ns string) int {
if !sonic_db_init {
Copy link
Collaborator

@qiluo-msft qiluo-msft Dec 15, 2023

Choose a reason for hiding this comment

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

sonic_db_init

swsscommon already has this check and init. #Closed

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this block removed, there is no value to define this function. We can remove the caller code to swsscommon function.

Copy link
Contributor Author

@ganglyu ganglyu Dec 15, 2023

Choose a reason for hiding this comment

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

https://github.com/sonic-net/sonic-swss-common/blob/master/common/dbconnector.cpp#L218-L227
swsscommon API like getDbInfo does not use initializeGlobalConfig, so we still need to initialize SonicDBConfig before using swsscommon API.

@@ -79,23 +80,3 @@ func TestGetDbMultiNs(t *testing.T) {
})
}

func TestOverrideDbConfigFile(t *testing.T) {
Copy link
Collaborator

@qiluo-msft qiluo-msft Dec 15, 2023

Choose a reason for hiding this comment

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

TestOverrideDbConfigFile

This UT is useful. Let's move it to swss-common repo? #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.

https://github.com/sonic-net/sonic-swss-common/blob/master/tests/main.cpp#L24-L34
swss-common has the unit test for nonexisting db config file.

if err != nil {
t.Fatalf("failed to get addr %v", err)
}
db, err := sdcfg.GetDbId("COUNTERS_DB", namespace)
Copy link
Collaborator

@qiluo-msft qiluo-msft Dec 15, 2023

Choose a reason for hiding this comment

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

:=

Do you want to use = instead of := ? #Closed

Copy link
Contributor Author

@ganglyu ganglyu Dec 16, 2023

Choose a reason for hiding this comment

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

":=" is used for declaration of "db".
If I use "=" instead, I need to declare "db" at first:
var db int
db, err = sdcfg.GetDbId("COUNTERS_DB", namespace)

@@ -145,7 +149,7 @@ func getPfcwdMap() (map[string]map[string]string, error) {
}

for _, key := range resp {
name := key[7:]
name := key[13:]
Copy link
Collaborator

@qiluo-msft qiluo-msft Dec 15, 2023

Choose a reason for hiding this comment

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

13

13

Where is 13 from? Add some code comment? #Closed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like Zain recently fixed a bug, but you branch is up-to-date. Could you rebase or merge master?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@sonic-net sonic-net deleted a comment from azure-pipelines bot Dec 16, 2023
@sonic-net sonic-net deleted a comment from mssonicbld Dec 16, 2023
qiluo-msft
qiluo-msft previously approved these changes Dec 18, 2023
Copy link
Collaborator

@FengPan-Frank FengPan-Frank left a comment

Choose a reason for hiding this comment

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

Refer below comment.

if err != nil {
return "", err
}
port, err := GetDbPort(db_name, ns)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we keep using err instead of new err variable here GetDbHostName() and GetDbPort()? since CatchException(&err) seems to be overwritten ambiguously?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CatchException should be removed.

@ganglyu ganglyu merged commit 752f1fc into sonic-net:master Dec 19, 2023
5 checks passed
faraazbrcm pushed a commit to faraazbrcm/sonic-gnmi that referenced this pull request Jan 8, 2024
Why I did it
sonic_db_config/db_config.go reads from database configuration json to get database configuration, but swsscommon has provided the same feature.

How I did it
Use swsscommon API to read database configuration.
Pending on below PR to clear database configuration for unit test.
sonic-net/sonic-swss-common#843

How to verify it
Run unit test.
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