Refine publish notifications #271

qiluo-msft · 2019-03-10T08:00:29Z

This is a competing PR to #258

I reverted #214 since it exposed unnecessary implementation details.

Problem statement is same as #258, so I shamelessly copied it here.

The typical use case that this PR is trying to deal with is related to route flapping.

Assuming such a scenario:
<1> 4K routes learned on an interface.

<2> interface down, 4k routes withdraw.
<3> fpmsyncd inserts 4k route keys into producerStateTabk keyset and generates 4k redis publish notifications

<4> Interface goes up

<4.A>
Interface may go up and learn the 4k routes back before orchagent has chance to pick up any notification and process the keyset. That is fine, change in #257 handled this case, no new redis publish notification will be created since the route keys already exist in producerStateTabk keyset and are to be processed.

<4.B>
Interface may go up and learn the 4k routes back after orchagent has processed a few notifications but not all the redis notifications (say, 1k notification processed), with the first notification, consumer in orchagent batch popped all the 4k route keys from producerStateTabk keyset, (orchagent has set popBatchSize as 8192), for the other 999 notifications processed, orchagent just call consumer_state_table_pops.lua 999 times without seeing any real data.
4k - 1k = 3k more redis notifications pending in redis queue now.

<5> Interface goes down
<5.A> If <4.A> was hit during previous interface up, good, no new redis publish notification will be generated

<5.B> If <4.B> was hit during previous interface up. 4K more redis notifications put into the redis queue.
<6> Interface goes up, loop back to <4>

The exact scenarios vary, but from <4> to <6>, number of pending notifications keeps increasing.

The issue is that orchagent may be abled to handle all the route changes with the batch processing (popBatchSize = 8192), redis publish notification was generated for each key and orchagent has to respond to each of the notifications with current select framework though most of them bring no value.

The implementation proposed here follows some design principles.

Generate one and only one Redis publish notification (channel message) when there is one data in the key set. So the number of messages and keys are the same, and we can ensure to decrease the same amount during pop/pops()
The only exception to above is when producer calls clear(). The channel messages could not be recalled.
Some users will prefer pop(), and others may prefer pops() to pop a batch. But no one will use both for the same consumer table
Consumer will never publish to channel, ie. create channel message

qiluo-msft · 2019-03-10T08:02:05Z

@jipanyang @zhenggen-xu As we offline discussed I submit this PR. Please help review. Thanks for your solution and inspiration. #Closed

common/redisselect.h

jipanyang

Compared with the existing implementation, changes in this PR optimized the processing of notification at Select() of consumerStateTable side.

We need to confirm if it is good enough to cope with the route flapping case seen previously. @zhenggen-xu

jipanyang · 2019-04-17T20:34:08Z

common/redisselect.cpp

+         * This is no big harm since all the late messages will be selected and nothing popped
+         * when the channel is not completely busy.
+         */
+        n -= m_queueLength;


Any usage of "n" after this operation? #Resolved

No. It will be optimized out by compiler. It may help readability a little bit.

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

No. It will be optimized out by compiler. It may help readability a little bit.

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

I think it causes more confusion than helping readability. #Resolved

Removed.

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

jipanyang · 2019-04-18T04:40:02Z

common/redisselect.cpp

+     * in the select-pop(s) pattern
+     */
+    if (n <= 0)
+        n = 1;


In what scenario n <= 0?

#Resolved

I am trying to prevent any logic error outside the scope of RedisSelect. Considering the case if there are redundant notifications but there is no data popped, caller may call discard(0), and here the function consumes one message in the queue if there is any. discard(negative) is not for protection purpose and should be not in a true case.

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

jipanyang · 2019-04-18T04:41:22Z

common/redisselect.cpp

+         * This is no big harm since all the late messages will be selected and nothing popped
+         * when the channel is not completely busy.
+         */
+        n -= m_queueLength;


No. It will be optimized out by compiler. It may help readability a little bit.

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

I think it causes more confusion than helping readability. #Resolved

…() (sonic-net#214)" This reverts commit c5d5434.

Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>

jipanyang

Looks good to me.

lguohan · 2019-05-24T08:42:24Z

retest this please

pavel-shirshov

Have we made any tests checking the new implementation is faster than previous?
Other as comments

pavel-shirshov · 2019-05-24T20:01:47Z

common/consumertablebase.cpp

+// Override with an empty body, and subclasses will explicitly discard messages in the channel
+void ConsumerTableBase::updateAfterRead()
+{
+}


I don't understand why do we need this?
The base class already has an empty body

pavel-shirshov · 2019-05-24T20:04:36Z

tests/redis_subscriber_state_ut.cpp

-        r = cs.isQueueEmpty();
-        EXPECT_TRUE(r);
-    }
-}


Why this test was removed?

pavel-shirshov · 2019-05-24T20:10:54Z

common/consumertablebase.cpp

+bool ConsumerTableBase::hasCachedData()
+{
+    return true;
+}


The base class has the same implementation. It doesn't need to be implemented here.

pavel-shirshov · 2019-05-24T20:12:00Z

common/subscriberstatetable.cpp

@@ -88,7 +88,7 @@ bool SubscriberStateTable::hasData()

 bool SubscriberStateTable::hasCachedData()
 {
-    return m_buffer.size() > 1 || m_keyspace_event_buffer.size() > 1;
+    return true;


It's default implementation in the base class. We could remove whole method.

qiluo-msft changed the title ~~Qiluo/producer state publish2~~ Refine publish notifications Mar 10, 2019

qiluo-msft requested review from pavel-shirshov, lguohan and kcudnik and removed request for pavel-shirshov and lguohan March 10, 2019 08:03

kcudnik reviewed Mar 10, 2019

View reviewed changes

common/redisselect.h Show resolved Hide resolved

kcudnik previously approved these changes Mar 10, 2019

View reviewed changes

jipanyang reviewed Apr 17, 2019

View reviewed changes

jipanyang reviewed Apr 18, 2019

View reviewed changes

qiluo-msft added 4 commits April 24, 2019 22:36

Implement more accurate channel message discarding

152aed2

Revert "Warm reboot: Add support for bool Select::hasCachedSelectable…

628b952

…() (sonic-net#214)" This reverts commit c5d5434.

(comments)

b2410ab

Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>

Remove usused var

74778c0

Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>

qiluo-msft force-pushed the qiluo/producerState_publish2 branch from 40e9e79 to 74778c0 Compare April 25, 2019 00:57

jipanyang approved these changes Apr 25, 2019

View reviewed changes

pavel-shirshov suggested changes May 24, 2019

View reviewed changes

qiluo-msft dismissed kcudnik’s stale review via 74778c0 July 29, 2021 03:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine publish notifications #271

Refine publish notifications #271

qiluo-msft commented Mar 10, 2019

qiluo-msft commented Mar 10, 2019 •

edited

Loading

jipanyang left a comment

jipanyang Apr 17, 2019 •

edited by qiluo-msft

Loading

qiluo-msft Apr 18, 2019

jipanyang Apr 18, 2019 •

edited by qiluo-msft

Loading

qiluo-msft Apr 25, 2019

jipanyang Apr 18, 2019 •

edited by qiluo-msft

Loading

qiluo-msft Apr 25, 2019 •

edited

Loading

jipanyang Apr 18, 2019 •

edited by qiluo-msft

Loading

jipanyang left a comment

lguohan commented May 24, 2019

pavel-shirshov left a comment

pavel-shirshov May 24, 2019

pavel-shirshov May 24, 2019

pavel-shirshov May 24, 2019

pavel-shirshov May 24, 2019

Refine publish notifications #271

Are you sure you want to change the base?

Refine publish notifications #271

Conversation

qiluo-msft commented Mar 10, 2019

qiluo-msft commented Mar 10, 2019 • edited Loading

jipanyang left a comment

Choose a reason for hiding this comment

jipanyang Apr 17, 2019 • edited by qiluo-msft Loading

Choose a reason for hiding this comment

qiluo-msft Apr 18, 2019

Choose a reason for hiding this comment

jipanyang Apr 18, 2019 • edited by qiluo-msft Loading

Choose a reason for hiding this comment

qiluo-msft Apr 25, 2019

Choose a reason for hiding this comment

jipanyang Apr 18, 2019 • edited by qiluo-msft Loading

Choose a reason for hiding this comment

qiluo-msft Apr 25, 2019 • edited Loading

Choose a reason for hiding this comment

jipanyang Apr 18, 2019 • edited by qiluo-msft Loading

Choose a reason for hiding this comment

jipanyang left a comment

Choose a reason for hiding this comment

lguohan commented May 24, 2019

pavel-shirshov left a comment

Choose a reason for hiding this comment

pavel-shirshov May 24, 2019

Choose a reason for hiding this comment

pavel-shirshov May 24, 2019

Choose a reason for hiding this comment

pavel-shirshov May 24, 2019

Choose a reason for hiding this comment

pavel-shirshov May 24, 2019

Choose a reason for hiding this comment

qiluo-msft commented Mar 10, 2019 •

edited

Loading

jipanyang Apr 17, 2019 •

edited by qiluo-msft

Loading

jipanyang Apr 18, 2019 •

edited by qiluo-msft

Loading

jipanyang Apr 18, 2019 •

edited by qiluo-msft

Loading

qiluo-msft Apr 25, 2019 •

edited

Loading

jipanyang Apr 18, 2019 •

edited by qiluo-msft

Loading