[Flex_counter]: Remove poll interval from db key by sihuihan88 · Pull Request #289 · sonic-net/sonic-sairedis

sihuihan88 · 2018-01-25T03:24:59Z

Signed-off-by: Sihui Han [email protected]

sihuihan88 · 2018-01-25T03:26:07Z

sihuihan88 · 2018-01-25T18:28:59Z

retest this please

marian-pritsak · 2018-01-26T11:54:04Z

syncd/syncd.cpp

    std::shared_ptr<swss::NotificationConsumer> restartQuery = std::make_shared<swss::NotificationConsumer>(dbAsic.get(), "RESTARTQUERY");
-    std::shared_ptr<swss::ConsumerStateTable> flexCounterState = std::make_shared<swss::ConsumerStateTable>(dbFlexCounter.get(), PFC_WD_STATE_TABLE);
-    std::shared_ptr<swss::ConsumerStateTable> flexCounterPlugin = std::make_shared<swss::ConsumerStateTable>(dbFlexCounter.get(), PLUGIN_TABLE);
+    std::shared_ptr<swss::ConsumerTable> pfcWdFlexCounter = std::make_shared<swss::ConsumerTable>(dbFlexCounter.get(), FLEX_PFC_WD_TABLE);


Flex counter was designed in a way that it's agnostic to its clients. In the case when we'll need to implement, for example, another watchdog, flex counter's code will need to be modified. I don't think that removing polling interval from key with a cost of introducing this dependency is a good thing to do.

sihuihan88 · 2018-01-30T18:56:04Z

retest this please

sihuihan88 · 2018-01-30T20:45:17Z

retest this please

sihuihan88 · 2018-01-31T18:50:24Z

@marian-pritsak Could you help to review? Thanks :)

lguohan · 2018-02-05T18:13:15Z

            SWSS_LOG_ERROR("Object type not supported");

minor comments: better to print out the objectType and the field to give more debug information.

Refers to: syncd/syncd.cpp:2682 in 103a7f8. [](commit_id = 103a7f8, deletion_comment = False)

lguohan · 2018-02-05T18:13:54Z

syncd/syncd_flex_counter.cpp

+
+    FlexCounter &fc = getInstance(instanceId);
+    fc.m_pollInterval = pollInterval;
+    // fc.startFlexCounterThread();


// fc.startFlexCounterThread(); [](start = 1, length = 34)

can we remove this?

lguohan · 2018-02-05T18:20:44Z

syncd/syncd_flex_counter.h

-        FlexCounter(uint32_t pollInterval);
-        static FlexCounter& getInstance(uint32_t pollInterval);
-        static void removeInstance(uint32_t pollInterval);
+        FlexCounter(std::string instanceId);


instanceId [](start = 32, length = 10)

can you explain this instanceId concept? I am not familiar with it.

It looks like the instanceId is the flex counter group name, so maybe change to groupname is more clear?

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

Each use case group will be an instance in flex counter and each instance will have a unique instance id which is set as group name for now. The group name is obtained from database.

lguohan · 2018-02-05T18:28:14Z

syncd/syncd_flex_counter.cpp

-        fc.m_queueCounterIdsMap.erase(counterIter);
-        found = true;
+        SWSS_LOG_ERROR("Trying to remove nonexisting queue counter Ids 0x%lx", queueVid);
+        if (fc.m_queueCounterIdsMap.empty() && fc.m_portCounterIdsMap.empty() && fc.m_queueAttrIdsMap.empty())


fc.m_queueCounterIdsMap.empty() && fc.m_portCounterIdsMap.empty() && fc.m_queueAttrIdsMap.empty( [](start = 12, length = 96)

lots of duplicated code to check empty counter id, suggest to have a function to check it.

for example

IsCounterIdsMapEmpty()

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

lguohan · 2018-02-05T18:34:02Z

syncd/syncd_flex_counter.cpp

    }
 }

 void FlexCounter::removeQueue(


for removeQueue, I think you changed the logic.

In marian's code, he tries to remove the queue id both from m_queueCounterIdsMap and m_queueAttrIdsMap.

In your code, if the first one is not found, you will return and will not remove the queueid from the second map.

Why do you change the logic here? Is this intended? IMO, marian code is more correct.

Marian's new logic was added recently and current one is the previous version. I didn't rebase it before force push. Will update and be careful next time. Sorry for the inconvenience

lguohan · 2018-02-05T18:38:14Z

syncd/syncd.cpp

+            }
+            else
+            {
+                SWSS_LOG_ERROR("Object type not supported");


SWSS_LOG_ERROR("Object type not supported"); [](start = 16, length = 44)

should be field not supported, also please print out the actual field for debugging purpose.

lguohan · 2018-02-05T18:38:56Z

syncd/syncd.cpp

    swss::KeyOpFieldsValuesTuple kco;
    consumer.pop(kco);

    const auto &key = kfvKey(kco);


key [](start = 16, length = 3)

-> groupname

lguohan

overall the refactor looks straightforward, however there is still some surprises, especially the removequeue logic were rewritten which is not exactly the same as before. In the commit message, the author should highlight such change if it is intended, and the author should avoid such change if it is not intended.

Signed-off-by: Sihui Han <[email protected]>

sihuihan88 mentioned this pull request Jan 25, 2018

[PfcWd]: Make polling interval configurable sonic-net/sonic-swss#435

Merged

sihuihan88 force-pushed the dev/sihan/flex branch from 31fd3f3 to 28ec34a Compare January 25, 2018 18:58

marian-pritsak requested changes Jan 26, 2018

View reviewed changes

sihuihan88 force-pushed the dev/sihan/flex branch from 28ec34a to 103a7f8 Compare January 27, 2018 00:46

lguohan reviewed Feb 5, 2018

View reviewed changes

sihuihan88 force-pushed the dev/sihan/flex branch 4 times, most recently from 3193039 to 602ffbe Compare February 5, 2018 20:15

[Flex_counter]: Remove poll interval from db key

4cbd048

Signed-off-by: Sihui Han <[email protected]>

sihuihan88 force-pushed the dev/sihan/flex branch from 602ffbe to 4cbd048 Compare February 5, 2018 20:38

lguohan approved these changes Feb 7, 2018

View reviewed changes

lguohan merged commit dc38653 into sonic-net:master Feb 7, 2018

sihuihan88 deleted the dev/sihan/flex branch February 7, 2018 23:28

pettershao-ragilenetworks pushed a commit to pettershao-ragilenetworks/sonic-sairedis that referenced this pull request Nov 18, 2022

[Flex_counter]: Remove poll interval from db key (sonic-net#289)

b120a2e

Signed-off-by: Sihui Han <[email protected]>

Conversation

sihuihan88 commented Jan 25, 2018

Uh oh!

sihuihan88 commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sihuihan88 commented Jan 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sihuihan88 commented Jan 30, 2018

Uh oh!

sihuihan88 commented Jan 30, 2018

Uh oh!

sihuihan88 commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lguohan commented Feb 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sihuihan88 Feb 5, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sihuihan88 Feb 5, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sihuihan88 commented Jan 25, 2018 •

edited

Loading

sihuihan88 commented Jan 31, 2018 •

edited

Loading

sihuihan88 Feb 5, 2018 •

edited

Loading

sihuihan88 Feb 5, 2018 •

edited

Loading