[orchagent]: Fix bug that Executor does not get correct priority saved in m_selectable

[jihaix]

1、What I did
changes in sonic-swss-common: make member function Select::getPri() to be virtual
changes in sonic-swss: override getPri() in class Executor to return priority of m_selectables

2、Why I do it
Select:cmp using priority got by function getPri() to sort order of event. But class Select's member function getPri() is not virtual，so that class Executor can't not override getPri() to return priority saved in m_selectable. Class Executor is just a container of pointer m_selectable with type Select *. We should get priority saved in m_selectable rather than 0, which is default structed by Executor's base class Select. Thinking of below situation , pointer Select * s point to object of type Executor, s -> getPri() will always return priority of 0

3、How I verified it
Debug & Test， show priority of Executor Object

[jihaix]

pull request in sonic-swss-common
sonic-net/sonic-swss-common#290

[pavel-shirshov]

before we hade this method how everything worked?
What getPri it was used?

[jihaix]

1 . This bug only found in class Executor used for orch. It worked well because most orch is not sensitive to priority, namely the process order of TABLE .
pull request in sonic-swss-common sonic-net/sonic-swss-common#290

My next pull request will show how priority influence the aclorch . aclorch works well in such condition: ACL_TABLE is processed firstly , ACL_RULE is processed secondly.
Class Executor is special, it's derived class of selectable , but it contains a Selectable pointer m_selecrtable that makes sense in function Select::select(). Priority of Executor's base class is always 0. Thinking below condition, Pointer Selectable * s point to an Executor object, s->getPri() will return priority derived from Selectable rather than priority of m_selectable.

2、 getPri was used by Select::(std::set<Selectable *, Select::cmp> m_ready) .
when event happends, Select::poll_descriptors will insert Selectable pointer into m_ready, Select::cmp use priority to decide the process order of Selectable pointer. Selectable pointer with higher priority will be processed earlier。

int Select::poll_descriptors(Selectable **c, unsigned int timeout)
{
......
     for (int i = 0; i < ret; ++i)
    {
        int fd = events[i].data.fd;
        Selectable* sel = m_objects[fd];
        sel->readData();
        m_ready.insert(sel);  //insert Selectable* sel to be processed
    }
......

     if (!m_ready.empty())
    {
        auto sel = *m_ready.begin(); //  sel with highest priority

        *c = sel;

        m_ready.erase(sel);
......
}

[pavel-shirshov]

In this case I'd suggest to fix the design of Executor, which at the same time using inheritance and composition which is not correct. Should we disable inheritance and use compassion everywhere?

[jihaix]

You are right, the design of Executor looks strange, but I think it's designed for two reasons:
1、 Executor is a container of pointer Selectable and the pointer is initialized by derived class pointer of Selectable, such as SubscriberStateTable, SubscriberStateTable. In this way, Executor can process event of derived class pointer of Selectable , such as readData from subscribe table
2、 Executor need to work as a selectable， so that it can be handled by Select::select()。Executor need to be selected by Select::select so that Executor::execute() can be exec to process doTask of orch
This may be a elegant and simple design that meets the requirements mentioned above.

So I think we can do it in two steps：
Firstly, fix the bug of priority under current design
Secondly, discuss whether it is necessary to modify the design of class Executor. After all, class Executor is wildly used in orchagent, it's better to be cautious.

[stcheng]

@qiluo-msft please check

[jihaix]

retest this please，there will be no mirror since Selectable:getPri() has been modified to be virtual in sonic-swss-common

[stcheng]

retest this please

[lguohan]

retest this please

[stcheng]

retest this please

[lguohan]

retest this please

[pavel-shirshov]

@lguohan The vs test is always falling for this change.
I suggested to change the design of Executor, because currently the design is obscured:

1. Executor has a pointer to Selectable which it represent
2. Executor inherited from Selectable.
   I'd suggest to remove inheritance and return the pointer to the Selectable (item 1), when we need it.

[jihaix]

@lguohan The vs test is always falling for this change.
I suggested to change the design of Executor, because currently the design is obscured:

1. Executor has a pointer to Selectable which it represent
2. Executor inherited from Selectable.
   I'd suggest to remove inheritance and return the pointer to the Selectable (item 1), when we need it.

1、The current design use Decorator Pattern, In object-oriented programming, the decorator pattern is a design pattern that allows behavior to be added to an individual object, dynamically, without affecting the behavior of other objects from the same class. Executor corresponds Decorator, Selectable corresponds Component, SubscribeTable corresponds ConcreteComponent.

2、 Initial Executor with priority of SubscribeTable is not a good idea. In this way, there will be two priority that is equal, namely Executor.priority == SubscribeTable.priority. This is pretty strange.

[pavel-shirshov]

@jihaix Can you please fix the vs test? We can't merge the code which breaks the tests.

[jihaix]

@jihaix Can you please fix the vs test? We can't merge the code which breaks the tests.

Okay， I'll fix the vs test soon

[jihaix]

check #1022 please

[stcheng]

could you merge these two pull requests into one?

[pavel-shirshov]

These pull requests are the same. I don't see any differences.