Add map of database ID -> table name separator; No longer pass table name separator when constructing Table objects #190

jleveque · 2018-03-22T21:43:40Z

Previously, SubscriberStateTable was hardcoded to use CONFIGDB_TABLE_NAME_SEPARATOR, which meant that one could only subscribe to notifications from databases which used "|" as a table name separator. If you subscribed to a database which used ":" as a table name separator, you would never receive notifications.

This change creates a constant map of database ID to table name separator, and chooses the appropriate table name separator for a table based on the DB one is connecting to. While fixing the above problem, I also simplified the method calls for instantiating Table objects, as one no longer needs to pass in the table separator. This will also prevent table name separator mismatches in the future.

Also changed name of DEFAULT_TABLE_NAME_SEPARATOR to LEGACY_TABLE_NAME_SEPARATOR and CONFIGDB_TABLE_NAME_SEPARATOR to NEW_TABLE_NAME_SEPARATOR

Also define TEST_DB at index 15, the largest index supported by default Redis config, for use in unit tests.

NOTE: This PR will require related changes in sonic-swss, sonic-sairedis and sonic-buildimage repos. I will submit those PRs shortly.

…name separator when constructing table objects

… config

qiluo-msft · 2018-03-22T23:15:10Z

Please check the unit test failure. #Closed

qiluo-msft · 2018-03-22T23:19:44Z

common/redispipeline.h

@@ -92,6 +92,11 @@ class RedisPipeline {
        return m_remaining;
    }

+    DBConnector *getDbConnector()


getDbConnector [](start = 17, length = 14)

The m_db is for internal use by design. It is carefully appended and popped to make pipeline working. Do not let outside use interleaves with internal use. #Closed

Latest commit now exposes the DB ID of m_db, not m_db itself. #Closed

m_db should never be NULL. you may use assert, if you want.

In reply to: 176623397 [](ancestors = 176623397)

Removed NULL check. #Closed

qiluo-msft · 2018-03-22T23:20:17Z

common/schema.h

@@ -13,6 +13,7 @@ namespace swss {
 #define PFC_WD_DB       5
 #define FLEX_COUNTER_DB 5
 #define STATE_DB        6
+#define TEST_DB         15


15 [](start = 24, length = 2)

Is there a redis constant we could refer? #Closed

No constant that I could find, but I found that you can query the number of databases using the CONFIG GET databases command:

# redis-cli CONFIG GET databases 1) "databases" 2) "16"

However, with this info, we can't define a constant like TEST_DB. We would need a getter method which returns the test DB id as (num_databases -1).

I still believe defining TEST_DB here with the other DB IDs is an improvement over the previous solution of #define TEST_VIEW (7) in tests/redis_ut.cpp. Having to remember to increment that value when adding a new DB is onerous.

Originally TEST_VIEW is only used in unit test. So please leave any related constant there.

In reply to: 176849657 [](ancestors = 176849657)

It needs to be known here because it needs to be included in the table name separator map. Otherwise, the TableBase constructor will throw an exception, unless we simply use an existing DB ID for unit tests.

As we discussed, other developers may need the freedom to experiment in new DB id without it's allocated formally in swss-common library. So the requirement for out-of-range DB id should be supported.

qiluo-msft · 2018-03-22T23:22:22Z

common/table.h

-#define DEFAULT_TABLE_NAME_SEPARATOR    ":"
-#define CONFIGDB_TABLE_NAME_SEPARATOR   "|"
+#define LEGACY_TABLE_NAME_SEPARATOR  ":"
+#define NEW_TABLE_NAME_SEPARATOR     "|"


NEW_TABLE_NAME_SEPARATOR [](start = 8, length = 24)

NEW_ is not a good prefix for long term project. How about SAFE_TABLE_NAME_SEPARATOR, or DEFAULT_ if it is default for future. #Closed

Suggest to move into class constant member, and add a getter function.

In reply to: 176601615 [](ancestors = 176601615)

qiluo-msft · 2018-03-22T23:23:52Z

common/table.h

@@ -18,8 +18,11 @@

 namespace swss {

-#define DEFAULT_TABLE_NAME_SEPARATOR    ":"
-#define CONFIGDB_TABLE_NAME_SEPARATOR   "|"
+#define LEGACY_TABLE_NAME_SEPARATOR  ":"


LEGACY_TABLE_NAME_SEPARATOR [](start = 8, length = 27)

Is it possible to migrate from legacy to new completely? How much effort needed? #Closed

I agree we should migrate completely to using only one separator (new) everywhere. I'm not sure of the total amount of work required for that. This is a question for @lguohan.

However, the changes I've made in this PR will help make that transition easier, by greatly reducing the number of references that will need to be updated.

qiluo-msft

As comments.

qiluo-msft · 2018-03-23T00:56:36Z

common/table.h

+#define NEW_TABLE_NAME_SEPARATOR     "|"
+
+// Mapping of DB ID to table name separator string
+typedef std::map<int, std::string> TableNameSeparatorMap;


TableNameSeparatorMap [](start = 35, length = 21)

suggest to keep it as a class private member. #Closed

kcudnik · 2018-03-23T23:58:44Z

This will probably break a lot of stuff in sairedis since sairedis is using hardcoded table separator also, but as string, for example "Table:*" when its lists all the tables, and it may be not easy task to fix all those places
maybe tests will catch some of those which will be used

jleveque · 2018-03-24T00:11:37Z

@kcudnik: This PR shouldn't cause any issues with existing hardcoded strings containing a colon as a separator (like Table:*), as I haven't changed which DBs use which separators. However, those will be an issue once we decide to change all separators across all DBs to use the pipe character instead. It sounds like that will be a big job.

FYI, there was only one reference in sairedis that explicitly referenced DEFAULT_TABLE_NAME_SEPARATOR, and I have removed that to work with this PR here: https://github.com/Azure/sonic-sairedis/pull/309/files

lguohan · 2018-03-24T18:19:47Z

common/dbconnector.h

@@ -35,7 +35,7 @@ class DBConnector

 private:
    redisContext *m_conn;
-    int m_db;
+    int m_id;


rename it as m_dbid to align with your function name getDbId()

lguohan · 2018-03-24T18:23:29Z

common/table.cpp

-Table::Table(DBConnector *db, string tableName, string tableSeparator)
-    : Table(new RedisPipeline(db, 1), tableName, tableSeparator, false)
+const std::string TableBase::LEGACY_TABLE_NAME_SEPARATOR = ":";
+const std::string TableBase::NEW_TABLE_NAME_SEPARATOR = "|";


We do not need classify them as legacy or new, can we use TABLE_NAME_SEPARATOR_COLON, TABLE_NAME_SEPARATOR_VBAR. We can add comments to to say we are going to deprecate the COLON separator.

lguohan · 2018-03-24T18:26:07Z

how do you plan to deal with swsssdk the python version? If we incorporate your changes, do we need any change in swsssdk or we are good?

lguohan · 2018-03-24T18:28:11Z

I think it is better merge this after March release.

lguohan · 2018-03-24T18:29:32Z

in current db, do we have cases that use different separator in one table db? If not, i think this approach is ok.

lguohan

as comments.

jleveque · 2018-03-26T18:18:57Z

@lguohan:

how do you plan to deal with swsssdk the python version? If we incorporate your changes, do we need any change in swsssdk or we are good?

This change should have no impact on swsssdk, as swsssdk doesn't reference swss-common. It uses the Python Redis library directly, and thus is completely independent of swss-common.

However, this change will make it possible to use the new Python bindings of swss-common everywhere we currently use swsssdk, effectively allowing us to deprecate and eventually remove swsssdk altogether.

in current db, do we have cases that use different separator in one table db? If not, i think this approach is ok.

From my investigation, it appears as though there is no mixing of separators within any DBs. Each database consistently uses only one separator.

jleveque added 2 commits March 21, 2018 23:41

Add map of database ID -> table name separator; No longer pass table …

d5e438f

…name separator when constructing table objects

Add TEST_DB at index 15, the largest index supported by default Redis…

89c6c72

… config

jleveque added the Enhancement label Mar 22, 2018

jleveque self-assigned this Mar 22, 2018

jleveque requested review from lguohan, qiluo-msft and kcudnik March 22, 2018 21:43

This was referenced Mar 22, 2018

Align with swss-common table name separator changes sonic-net/sonic-swss#454

Merged

Align with swss-common table name separator changes sonic-net/sonic-sairedis#309

Merged

qiluo-msft reviewed Mar 22, 2018

View reviewed changes

qiluo-msft requested changes Mar 22, 2018

View reviewed changes

Fix unit tests

fcac4e2

qiluo-msft reviewed Mar 23, 2018

View reviewed changes

jleveque added 2 commits March 23, 2018 01:26

RedisPipeline exposes DB ID instead of DBConnector member

81dfbb0

Remove NULL check

b47ace8

Address review comments

b248e96

lguohan reviewed Mar 24, 2018

View reviewed changes

lguohan requested changes Mar 24, 2018

View reviewed changes

kcudnik approved these changes Mar 26, 2018

View reviewed changes

Rename m_id -> m_dbId

c42167a

Rename table name separator constants

5247f13

lguohan approved these changes Apr 10, 2018

View reviewed changes

jleveque merged commit 8213777 into sonic-net:master Apr 10, 2018

jleveque deleted the table_name_separators branch April 10, 2018 23:05

jleveque mentioned this pull request Apr 11, 2018

Incorporate swss-common Table Name Separator Changes sonic-net/sonic-buildimage#1584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add map of database ID -> table name separator; No longer pass table name separator when constructing Table objects #190

Add map of database ID -> table name separator; No longer pass table name separator when constructing Table objects #190

jleveque commented Mar 22, 2018

qiluo-msft commented Mar 22, 2018 •

edited

Loading

qiluo-msft Mar 22, 2018 •

edited

Loading

jleveque Mar 23, 2018 •

edited by qiluo-msft

Loading

qiluo-msft Mar 23, 2018

jleveque Mar 23, 2018 •

edited by qiluo-msft

Loading

qiluo-msft Mar 22, 2018 •

edited

Loading

jleveque Mar 23, 2018 •

edited

Loading

qiluo-msft Mar 23, 2018

jleveque Mar 23, 2018 •

edited

Loading

qiluo-msft Mar 23, 2018

qiluo-msft Mar 22, 2018 •

edited

Loading

qiluo-msft Mar 23, 2018

qiluo-msft Mar 22, 2018 •

edited

Loading

jleveque Mar 23, 2018

qiluo-msft left a comment

qiluo-msft Mar 23, 2018 •

edited

Loading

kcudnik commented Mar 23, 2018

jleveque commented Mar 24, 2018

lguohan Mar 24, 2018

lguohan Mar 24, 2018

lguohan commented Mar 24, 2018

lguohan commented Mar 24, 2018

lguohan commented Mar 24, 2018

lguohan left a comment

jleveque commented Mar 26, 2018 •

edited

Loading

Add map of database ID -> table name separator; No longer pass table name separator when constructing Table objects #190

Add map of database ID -> table name separator; No longer pass table name separator when constructing Table objects #190

Conversation

jleveque commented Mar 22, 2018

qiluo-msft commented Mar 22, 2018 • edited Loading

qiluo-msft Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

jleveque Mar 23, 2018 • edited by qiluo-msft Loading

Choose a reason for hiding this comment

qiluo-msft Mar 23, 2018

Choose a reason for hiding this comment

jleveque Mar 23, 2018 • edited by qiluo-msft Loading

Choose a reason for hiding this comment

qiluo-msft Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

jleveque Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Mar 23, 2018

Choose a reason for hiding this comment

jleveque Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Mar 23, 2018

Choose a reason for hiding this comment

qiluo-msft Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Mar 23, 2018

Choose a reason for hiding this comment

qiluo-msft Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

jleveque Mar 23, 2018

Choose a reason for hiding this comment

qiluo-msft left a comment

Choose a reason for hiding this comment

qiluo-msft Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

kcudnik commented Mar 23, 2018

jleveque commented Mar 24, 2018

lguohan Mar 24, 2018

Choose a reason for hiding this comment

lguohan Mar 24, 2018

Choose a reason for hiding this comment

lguohan commented Mar 24, 2018

lguohan commented Mar 24, 2018

lguohan commented Mar 24, 2018

lguohan left a comment

Choose a reason for hiding this comment

jleveque commented Mar 26, 2018 • edited Loading

qiluo-msft commented Mar 22, 2018 •

edited

Loading

qiluo-msft Mar 22, 2018 •

edited

Loading

jleveque Mar 23, 2018 •

edited by qiluo-msft

Loading

jleveque Mar 23, 2018 •

edited by qiluo-msft

Loading

qiluo-msft Mar 22, 2018 •

edited

Loading

jleveque Mar 23, 2018 •

edited

Loading

jleveque Mar 23, 2018 •

edited

Loading

qiluo-msft Mar 22, 2018 •

edited

Loading

qiluo-msft Mar 22, 2018 •

edited

Loading

qiluo-msft Mar 23, 2018 •

edited

Loading

jleveque commented Mar 26, 2018 •

edited

Loading