[TableBase] Make channel name from both table name and database ID by jimmyzhai · Pull Request #568 · sonic-net/sonic-swss-common

jimmyzhai · 2021-12-17T10:26:13Z

To overload member function getChannelName with parameter tag
To use dbId as tag and call getChannelName in producer/consumertable and producer/consumerstatetable

* remove the deprecated tableNameSeparatorMap, always get separator from SonicDBConfig

qiluo-msft · 2021-12-18T04:02:40Z

common/table.h

 class TableBase {
 public:
-#ifndef SWIG
-    __attribute__((deprecated))


deprecated

Please don't remove deprecated attribute. The reason is we are using dbName to identify a database instead of dbId. And the deprecated ctor is only used in SWIG, and then used by old test code, which we did not migrate completely. #Closed

Looks dbId is widely used to identify a database, and both Id and Name are primary properties of database. In database domain hierarchy, table always belongs to one database. To add dbId member in class TableBase is meaningful to indicate in which database the table is. In ctor internal, table separator is initialized to the separator of database dbId.

qiluo-msft · 2021-12-18T04:17:09Z

common/table.h

    }

-    std::string getChannelName() { return m_tableName + "_CHANNEL"; }
+    std::string getChannelName() { return m_tableName + "_CHANNEL"


getChannelName

The dbId is a concept out of TableBase class.
Could you add new parameter dbId, and do not use a member? #Closed

As above reply, I thinking adding dbId as a member is much reasonable.

qiluo-msft

As comments

common/table.h

common/table.cpp

common/producertable.cpp

qiluo-msft · 2022-01-06T07:08:43Z

common/notificationproducer.cpp

@@ -1,7 +1,7 @@
 #include "notificationproducer.h"

 swss::NotificationProducer::NotificationProducer(swss::DBConnector *db, const std::string &channel):


NotificationProducer

Do you need to change NotificationConsumer accordingly?

Updated. Hold on the change of channel name tagged dbId for Notification Producer/Consumer. Some lua scripts like pfc_restore.lua, pfe_detect are using redis PUBLISH command directly with hardcode channel name.

What I did In p4rt test scripts, bind response consumer to appl_state_db for consistency. Why I did it In p4orch, the relating notification producer for table P4RT_TABLE is on database appl state db How I verified it Run vstest Details if related notification channel name would be tagged dbId - sonic-net/sonic-swss-common#568

jimmyzhai · 2022-01-11T01:33:03Z

/azp run

azure-pipelines · 2022-01-11T01:33:15Z

Azure Pipelines successfully started running 1 pipeline(s).

jimmyzhai · 2022-01-12T03:17:35Z

/azp run

azure-pipelines · 2022-01-12T03:17:47Z

Azure Pipelines successfully started running 1 pipeline(s).

jimmyzhai · 2022-01-12T09:10:36Z

/azp run

azure-pipelines · 2022-01-12T09:10:48Z

Azure Pipelines successfully started running 1 pipeline(s).

…se ID (sonic-net#568)" This reverts commit bd4d0ce.

…se ID (#568)" (#574) This reverts commit bd4d0ce.

…d database ID (#568)" (#574)" This reverts commit 14f068e.

…d database ID (#568)" (#574)" (#578) Reverts #574 The real cause is fixed at sonic-net/sonic-sairedis#995. Continue to commit bd4d0ce again.

What I did In p4rt test scripts, bind response consumer to appl_state_db for consistency. Why I did it In p4orch, the relating notification producer for table P4RT_TABLE is on database appl state db How I verified it Run vstest Details if related notification channel name would be tagged dbId - sonic-net/sonic-swss-common#568

jimmyzhai added 3 commits December 17, 2021 09:50

Include database ID in channel name

b97cbd5

* add m_dbId member in TableBase

d85acd3

* remove the deprecated tableNameSeparatorMap, always get separator from SonicDBConfig

Fix typo

cd0cf84

jimmyzhai requested a review from qiluo-msft December 17, 2021 10:36

qiluo-msft reviewed Dec 18, 2021

View reviewed changes

qiluo-msft requested changes Dec 18, 2021

View reviewed changes

qiluo-msft reviewed Dec 29, 2021

View reviewed changes

common/table.h Show resolved Hide resolved

qiluo-msft reviewed Dec 29, 2021

View reviewed changes

common/table.cpp Show resolved Hide resolved

jimmyzhai added 3 commits January 4, 2022 07:44

Add taggged getChannelName

4a2606f

Use tagged getChannelName in consumerstatetable

a2bb74f

Append channel name with dbName in notification producer/consumer

a26f581

qiluo-msft reviewed Jan 4, 2022

View reviewed changes

common/producertable.cpp Outdated Show resolved Hide resolved

Use dbId as tag

1664422

qiluo-msft reviewed Jan 6, 2022

View reviewed changes

qiluo-msft requested a review from kcudnik January 6, 2022 07:12

jimmyzhai mentioned this pull request Jan 7, 2022

[p4rt-test] Bind response consumer to appl_state_db sonic-net/sonic-swss#2105

Merged

qiluo-msft previously approved these changes Jan 12, 2022

View reviewed changes

revert the changes in notification producer/consumer

2734475

jimmyzhai dismissed qiluo-msft’s stale review via 2734475 January 13, 2022 07:47

qiluo-msft approved these changes Jan 13, 2022

View reviewed changes

jimmyzhai merged commit bd4d0ce into sonic-net:master Jan 14, 2022

qiluo-msft added a commit to qiluo-msft/sonic-swss-common that referenced this pull request Jan 15, 2022

Revert "[TableBase] Make channel name from both table name and databa…

9e43ceb

…se ID (sonic-net#568)" This reverts commit bd4d0ce.

qiluo-msft mentioned this pull request Jan 15, 2022

Revert "[TableBase] Make channel name from both table name and databa… #574

Merged

qiluo-msft added a commit that referenced this pull request Jan 15, 2022

Revert "[TableBase] Make channel name from both table name and databa…

14f068e

…se ID (#568)" (#574) This reverts commit bd4d0ce.

jimmyzhai added a commit that referenced this pull request Jan 24, 2022

Revert "Revert "[TableBase] Make channel name from both table name an…

d909eb4

…d database ID (#568)" (#574)" This reverts commit 14f068e.

mint570 mentioned this pull request Apr 7, 2022

Added batched APIs in ProducerStateTable. #588

Merged

		@@ -1,7 +1,7 @@
		#include "notificationproducer.h"

		swss::NotificationProducer::NotificationProducer(swss::DBConnector *db, const std::string &channel):

Conversation

jimmyzhai commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qiluo-msft Dec 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmyzhai Dec 28, 2021

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Dec 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmyzhai Dec 28, 2021

Choose a reason for hiding this comment

Uh oh!

qiluo-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qiluo-msft Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

jimmyzhai Jan 13, 2022

Choose a reason for hiding this comment

Uh oh!

jimmyzhai commented Jan 11, 2022

Uh oh!

azure-pipelines bot commented Jan 11, 2022

Uh oh!

jimmyzhai commented Jan 12, 2022

Uh oh!

azure-pipelines bot commented Jan 12, 2022

Uh oh!

jimmyzhai commented Jan 12, 2022

Uh oh!

azure-pipelines bot commented Jan 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jimmyzhai commented Dec 17, 2021 •

edited

Loading

qiluo-msft Dec 18, 2021 •

edited

Loading

qiluo-msft Dec 18, 2021 •

edited

Loading