Add ProducerStateTable temp view implementation and UT #247

taoyl-ms · 2018-11-10T00:41:29Z

See detail in design doc: https://github.com/Azure/SONiC/blob/master/doc/warm-reboot/view_switch.md

qiluo-msft · 2018-11-10T00:44:23Z

tests/redis_state_ut.cpp

@@ -29,18 +29,18 @@ static inline int getMaxFields(int i)

 static inline string key(int i)
 {
-    return string("key ") + to_string(i);


key [](start = 19, length = 4)

The original test data may be better, it is testing a key/field/value with blank char. #Closed

qiluo-msft · 2018-11-10T00:53:40Z

tests/redis_state_ut.cpp

+    hlen.format("HLEN %s", (c.getKeyName(key(0))).c_str());
+    RedisReply r6(&db, hlen, REDIS_REPLY_INTEGER);
+    EXPECT_EQ(r6.getReply<long long int>(), (long long int) maxNumOfFieldsNew);
+}


Suggestion test cases on corner flows if not covered already.

Double create:

create

create

apply

Double apply:

create

apply

apply

Delete all:

create

set many

delete all

apply

Empty view (ie. clear)

create

immediately apply
#Closed

Test cases added according to CR comments.

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

qiluo-msft · 2018-11-10T00:55:32Z

common/producer_state_table_apply_view.lua

@@ -0,0 +1,16 @@
+local arg_start = 2


Suggest add file header comments to describe the usage, such as parameters. #Closed

I put them in apply_view() function in producerstatetable.cpp. Do you think here will be a better place?

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

qiluo-msft · 2018-11-10T01:04:54Z

common/producerstatetable.cpp

+    TableDump currentState;
+    {
+        Table mainTable(m_pipe, getTableName(), false);
+        mainTable.dump(currentState);


dump [](start = 18, length = 4)

There is a comment "note that this function is not efficient"... #Closed

We're calling it only once here as well.

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

Even once, it is still not efficient.

In reply to: 232849738 [](ancestors = 232849738,232433590)

OK. I'll refine the implementation for better performance.

In reply to: 233584343 [](ancestors = 233584343,232849738,232433590)

qiluo-msft · 2018-11-10T01:08:15Z

common/producerstatetable.cpp

+        }
+        if (needDel) keysToDel.emplace_back(key);
+        if (needSet) keysToSet.emplace_back(key);
+        else m_tempViewState.erase(key);                               // If exactly match, no need to sync new state to StateHash in DB


follow existing coding style? #Closed

qiluo-msft

As comments.

jipanyang · 2018-11-10T05:24:30Z

common/producerstatetable.cpp

+    std::vector<std::string> keysToSet;
+    std::vector<std::string> keysToDel;
+
+    // Compare based on existing objects


It could be helpful if some debug level log is available to dump the data in current view and tmp view. #Resolved

This means to dump the entire table? Wouldn't there be too many logs?

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

it is one time thing, usually warm restart only. For debugging purpose, I think it is ok and helpful. #Resolved

jipanyang · 2018-11-10T05:55:19Z

common/producerstatetable.cpp

+    //   G   (String to be published to channel)
+    //   2   (Count of objects to set)
+    //   key_0
+    //   key_1


Excluding keys for del operation, It looks duplicate key data provided in KEYS and ARGV? #ByDesign

Technically yes, since all KEYS for StateHash has a corresponding key to set in KeySet. However, One of them has format '_SAMPLE:key_0' and is actual key of redis object, and another one has format 'key_0' and is not object key (therefore should not be passed in KEYS), I'll prefer to pass them as KEYS and ARGV seperatedly.

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

qiluo-msft · 2018-11-15T04:44:56Z

common/table_dump.lua

@@ -4,10 +4,11 @@ local res = {}
 for i,k in pairs(keys) do

   local skeys = redis.call("HKEYS", k)


skeys [](start = 9, length = 5)

skeys is not used and can be removed. #Resolved

Minor issue remains. Unblocked.

jipanyang

Overall the changes look good to me.

Could you add comments about the case as seen with fpmsyncd? They are not supported in current library.

 * FV1  {nexthop: 10.1.1.1, 10.1.1.2}
 * FV2  {nexthop: 10.1.1.2, 10.1.1.1}

jipanyang · 2018-11-16T19:24:49Z

common/producerstatetable.cpp

+
+    // Print content of current view and temp view as debug log
+    SWSS_LOG_DEBUG("View switch of table %s required.", getTableName().c_str());
+    SWSS_LOG_DEBUG("Objects in current view:");


Not so sure, would INFO level be better considering this is kind of critical data when debugging possible reconciliation issue, there is so much routine debug level messages in different modules as of today. #Resolved

jipanyang · 2018-11-16T19:26:29Z

common/producerstatetable.cpp

+        // Write to temp view instead of DB
+        for (const auto& iv: values)
+        {
+            m_tempViewState[key][fvField(iv)] = fvValue(iv);


Is case like below also ok? is that covered in unit test?

127.0.0.1:6379[4]> hgetall hello 1) "" 2) "" ``` #Resolved

Yes. Unit test already covered empty values.

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

Add ProducerStateTable temp view implementation and UT

0cdbef8

taoyl-ms requested a review from qiluo-msft November 10, 2018 00:41

qiluo-msft reviewed Nov 10, 2018

View reviewed changes

qiluo-msft previously requested changes Nov 10, 2018

View reviewed changes

jipanyang reviewed Nov 10, 2018

View reviewed changes

taoyl-ms added 4 commits November 12, 2018 23:37

Resolve PR comments

28cbaa7

Add log

1e78112

Improve table.dump() performance

28e49b9

Add test cases according to CR comments

8260d7b

qiluo-msft reviewed Nov 15, 2018

View reviewed changes

Resolve CR comments

94202e5

qiluo-msft approved these changes Nov 16, 2018

View reviewed changes

jipanyang approved these changes Nov 16, 2018

View reviewed changes

taoyl-ms added 2 commits November 16, 2018 20:15

Add comments as CR

9a9e6de

Use INFO level for table dump at apply_view

0acb3bf

taoyl-ms merged commit 9918ae6 into sonic-net:master Nov 16, 2018

jipanyang mentioned this pull request Jun 4, 2019

[warm restart assist] assume vector values could be reordered sonic-net/sonic-swss#921

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ProducerStateTable temp view implementation and UT #247

Add ProducerStateTable temp view implementation and UT #247

taoyl-ms commented Nov 10, 2018

qiluo-msft Nov 10, 2018 •

edited

Loading

qiluo-msft Nov 10, 2018 •

edited

Loading

taoyl-ms Nov 15, 2018

qiluo-msft Nov 10, 2018 •

edited

Loading

taoyl-ms Nov 10, 2018

qiluo-msft Nov 10, 2018 •

edited

Loading

taoyl-ms Nov 12, 2018

qiluo-msft Nov 14, 2018

taoyl-ms Nov 14, 2018

qiluo-msft Nov 10, 2018 •

edited

Loading

qiluo-msft left a comment

jipanyang Nov 10, 2018 •

edited by taoyl-ms

Loading

taoyl-ms Nov 12, 2018 •

edited

Loading

jipanyang Nov 13, 2018 •

edited by taoyl-ms

Loading

jipanyang Nov 10, 2018 •

edited by taoyl-ms

Loading

taoyl-ms Nov 12, 2018

qiluo-msft Nov 15, 2018 •

edited by taoyl-ms

Loading

jipanyang left a comment

jipanyang Nov 16, 2018 •

edited by taoyl-ms

Loading

jipanyang Nov 16, 2018 •

edited by taoyl-ms

Loading

taoyl-ms Nov 16, 2018

		@@ -4,10 +4,11 @@ local res = {}
		for i,k in pairs(keys) do

		local skeys = redis.call("HKEYS", k)

Add ProducerStateTable temp view implementation and UT #247

Add ProducerStateTable temp view implementation and UT #247

Conversation

taoyl-ms commented Nov 10, 2018

qiluo-msft Nov 10, 2018 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Nov 10, 2018 • edited Loading

Choose a reason for hiding this comment

taoyl-ms Nov 15, 2018

Choose a reason for hiding this comment

qiluo-msft Nov 10, 2018 • edited Loading

Choose a reason for hiding this comment

taoyl-ms Nov 10, 2018

Choose a reason for hiding this comment

qiluo-msft Nov 10, 2018 • edited Loading

Choose a reason for hiding this comment

taoyl-ms Nov 12, 2018

Choose a reason for hiding this comment

qiluo-msft Nov 14, 2018

Choose a reason for hiding this comment

taoyl-ms Nov 14, 2018

Choose a reason for hiding this comment

qiluo-msft Nov 10, 2018 • edited Loading

Choose a reason for hiding this comment

qiluo-msft left a comment

Choose a reason for hiding this comment

jipanyang Nov 10, 2018 • edited by taoyl-ms Loading

Choose a reason for hiding this comment

taoyl-ms Nov 12, 2018 • edited Loading

Choose a reason for hiding this comment

jipanyang Nov 13, 2018 • edited by taoyl-ms Loading

Choose a reason for hiding this comment

jipanyang Nov 10, 2018 • edited by taoyl-ms Loading

Choose a reason for hiding this comment

taoyl-ms Nov 12, 2018

Choose a reason for hiding this comment

qiluo-msft Nov 15, 2018 • edited by taoyl-ms Loading

Choose a reason for hiding this comment

jipanyang left a comment

Choose a reason for hiding this comment

jipanyang Nov 16, 2018 • edited by taoyl-ms Loading

Choose a reason for hiding this comment

jipanyang Nov 16, 2018 • edited by taoyl-ms Loading

Choose a reason for hiding this comment

taoyl-ms Nov 16, 2018

Choose a reason for hiding this comment

qiluo-msft Nov 10, 2018 •

edited

Loading

qiluo-msft Nov 10, 2018 •

edited

Loading

qiluo-msft Nov 10, 2018 •

edited

Loading

qiluo-msft Nov 10, 2018 •

edited

Loading

qiluo-msft Nov 10, 2018 •

edited

Loading

jipanyang Nov 10, 2018 •

edited by taoyl-ms

Loading

taoyl-ms Nov 12, 2018 •

edited

Loading

jipanyang Nov 13, 2018 •

edited by taoyl-ms

Loading

jipanyang Nov 10, 2018 •

edited by taoyl-ms

Loading

qiluo-msft Nov 15, 2018 •

edited by taoyl-ms

Loading

jipanyang Nov 16, 2018 •

edited by taoyl-ms

Loading

jipanyang Nov 16, 2018 •

edited by taoyl-ms

Loading