Split Orch class into two classes: OrchBase and Orch

[jipanyang]

Signed-off-by: Jipan Yang [email protected]

What I did

Split Orch class into two classes: Orch and OrchBase

Why I did it
OrchBase is to be used by both orchagent and configDB enforcers in cfgagent
How I verified it
jipan@0cc3061a68e8:/sonic/src/sonic-swss$ tests/tests
Running main() from gtest_main.cc
[==========] Running 9 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 8 tests from swssnet
[ RUN ] swssnet.copy1_v6
[ OK ] swssnet.copy1_v6 (0 ms)
[ RUN ] swssnet.copy1_v4
[ OK ] swssnet.copy1_v4 (0 ms)
[ RUN ] swssnet.copy2_v6
[ OK ] swssnet.copy2_v6 (0 ms)
[ RUN ] swssnet.copy2_v4
[ OK ] swssnet.copy2_v4 (0 ms)
[ RUN ] swssnet.copy3_v6
[ OK ] swssnet.copy3_v6 (0 ms)
[ RUN ] swssnet.copy3_v4
[ OK ] swssnet.copy3_v4 (0 ms)
[ RUN ] swssnet.subnet_v6
[ OK ] swssnet.subnet_v6 (0 ms)
[ RUN ] swssnet.subnet_v4
[ OK ] swssnet.subnet_v4 (0 ms)
[----------] 8 tests from swssnet (0 ms total)

[----------] 1 test from OrchBase
[ RUN ] OrchBase.test
Starting OrchBase testing

• Step 1. Provision TEST_CONFIG_DB and TEST_APP_DB
• Step 2. Verify TEST_RESULT_DB content
• Step 3. Flush TEST_RESULT_DB
• Step 4. Sync from TEST_CONFIG_DB and TEST_APP_DB
• Step 5. Verify TEST_RESULT_DB content
• Step 6. Clean TEST_CONFIG_DB and TEST_APP_DB
• Step 7. Verify TEST_RESULT_DB content is empty
  Done.
  [ OK ] OrchBase.test (11024 ms)
  [----------] 1 test from OrchBase (11024 ms total)

[----------] Global test environment tear-down
[==========] 9 tests from 2 test cases ran. (11024 ms total)
[ PASSED ] 9 tests.

Details if related

[jipanyang]

Part of the change is from #325 @oleksandrivantsiv for support of SubscriberStateTable in Orch class. Will update the code once 325 is merged.

[jipanyang]

This PR obsoletes #355 Add cfgOrch class

[lguohan]

why cannot orchagent class and cfgmgr share the same class? what is the fundamental difference between them?

[jipanyang]

One of the issues is those global variables used in existing Orch class:

extern mutex gDbMutex;
extern PortsOrch *gPortsOrch;
extern bool gSwssRecord;
extern ofstream gRecordOfs;
extern bool gLogRotate;
extern string gRecordFile;

cfgMgr is not interested in handling that. Having a base class with true common function simplifies the code.

[lguohan]

the last five of them are related to logs, I think cfgMgr will also do it at some time in the future. As to the portorch, it does not seem to be a big problem, we can probably remove it from orch class in the fture. dbMutex i think if cfgmgr will have multi-thread, it will need same mutex.

[jipanyang]

Is it a good design embedding so many global variables in a class? I understand that is the current situation in existing Orch class. Are we forced to inherit this practice in configDB enforcer implementation?

Also why do we want to implement everything in one clumsy class when the function separation and code could be more clear? Like the redis table related classes, a base class has the essential functions , new special functions may be implemented in derived classes.

[lguohan]

according to your description, I do not see a fundamental difference between config db mgr and orch class. for your concern, I think we can later refactor orch class.

[jipanyang]

You still want to deploy the original Orch class in configDB enforcers, with those global variables and everything in one single class?

Later often means never. What is blocking us from starting the refactoring now?

Don't want to pollute the new vlanmgr, intfmgr and switchmgr modules with those global variables. Having a OrchBase class with global variables stripped off and keeping only the essential basic functions, I see that a first step towards the final clean design.

What is your real concern here? Let's see if that could be addressed.

[jipanyang]

Orch class to be refactored in new PR.