Add refillToSync() into ConsumerBase to support warmboot. #2866
Conversation
There is no downside of add refillToSync(), I will review this PR and give comments. |
orchagent/orch.cpp
Outdated
| { | ||
| auto subTable = dynamic_cast<SubscriberStateTable *>(getSelectable()); | ||
| auto consumertable = dynamic_cast<ConsumerTableBase *>(getSelectable()); |
There was a problem hiding this comment.
Suggest move this to line 192 to avoid unnecessary convert, also because first if block returned, so line 192 can change from 'else if' to 'if'
#closed
orchagent/orch.cpp
Outdated
| else | ||
| { | ||
| // The consumerTable should be ZmqConsumerStateTable. | ||
| DBConnector db("APPL_DB", 0); |
There was a problem hiding this comment.
Please check if the consumerTable is ZmqConsumerStateTable and use getDbConnector() in ZmqConsumerStateTable:
The reason of this is because the APPL_DB may not in local and may not be 'APPL_DB', the design still under discussion, so if use DBConnector from ZMQ, the code can keep no change when design finalized.
#closed
c4973ea to
b120b29
Compare
orchagent/orch.cpp
Outdated
| auto table = Table(db, tableName); | ||
| return refillToSync(&table); | ||
| } | ||
| DBConnector db("APPL_DB", 0); | ||
| auto table = Table(&db, tableName); | ||
| return refillToSync(&table); |
There was a problem hiding this comment.
Why need this block? What is the class of getSelectable()? APPL_DB seems a magic choice.
There was a problem hiding this comment.
This is the default case if none of the above condition matches. Today, orchagent reads the entries from APPL DB after warm boot. I hear dash is different, but using APPL DB shouldn't be completely unreasonable.
Do you suggest to remove this default case handling? I can do that.
There was a problem hiding this comment.
I do not get "the class of getSelectable()?" in this case. If you do not either, let's remove this block.
|
@qiluo-msft to approve |
436f7c9 to
da66678
Compare
Change-Id: I29903d43d7b53ddcc92d836ec4e95439dfbda714
da66678 to
dec6fc1
Compare
|
The vs test failed. Not sure if it relates to this PR or not. |
no, there is a known issue with submodule PR for sai-redis. will merge this by next week. |
What I did
Add refillToSync() into ConsumerBase to support warmboot.
Why I did it
Add warmboot support for zmq consumer.
How I verified it
This will be useful when we migrate other orchs to use zmq.
Details if related
UT