[orchagent]: Remove global lock caused by notifications running in another thread #478
Conversation
orchagent/orch.h
Outdated
| @@ -152,7 +152,7 @@ class Orch | |||
|
|
|||
| /* Run doTask against a specific executor */ | |||
| virtual void doTask(Consumer &consumer) = 0; | |||
| virtual void doTask(NotificationConsumer &consumer) { } | |||
| virtual void doTask(NotificationConsumer &consumer, const std::string &name) { } | |||
There was a problem hiding this comment.
const std::string &name [](start = 56, length = 23)
can you add a comment on the name, what does this name use for?
There was a problem hiding this comment.
Comments added
There was a problem hiding this comment.
I've also renamed this parameter to more clear name
orchagent/fdborch.cpp
Outdated
| @@ -25,8 +26,14 @@ FdbOrch::FdbOrch(DBConnector *db, string tableName, PortsOrch *port) : | |||
| { | |||
| m_portsOrch->attach(this); | |||
| auto consumer = new NotificationConsumer(db, "FLUSHFDBREQUEST"); | |||
| auto fdbNotification = new Notifier(consumer, this); | |||
| auto fdbNotification = new Notifier(consumer, this, "FLUSHFDBREQUEST"); | |||
There was a problem hiding this comment.
fdbNotification [](start = 9, length = 15)
Confused about fdbNotification vs fdb_notification. What is the diff? #Closed
There was a problem hiding this comment.
Renamed all the variable names for notification related stuff
orchagent/fdborch.cpp
Outdated
| @@ -25,8 +26,14 @@ FdbOrch::FdbOrch(DBConnector *db, string tableName, PortsOrch *port) : | |||
| { | |||
| m_portsOrch->attach(this); | |||
| auto consumer = new NotificationConsumer(db, "FLUSHFDBREQUEST"); | |||
| auto fdbNotification = new Notifier(consumer, this); | |||
| auto fdbNotification = new Notifier(consumer, this, "FLUSHFDBREQUEST"); | |||
There was a problem hiding this comment.
Can we define the notification events as enumerations and derive the names from them? This way, we can put them in a common place so no hardcode of strings from different places.
Also, it would make the addition of future events implementation easier in case we want to implement the existing notification events or new added events.
There was a problem hiding this comment.
With enumerations we will have hardcoded enumerations.
I think in this case it's better to develop better abstraction than NotificationConsumer(). Currently it's connected too deep into Orch class (doTask methods for NotificationConsumer). We should decouple them from each other and implement doTask methods as a base class for Orch classes.
There was a problem hiding this comment.
I agree more changes is needed for a better abstraction than what we have today. but I am not sure if you are going to do it in this PR.
My point was, at least we should put the message types in a common place and reuse then on different places so you don't end up with mistype something and get run-time problem. enumerations will detect the errors during the build time not run time if you mistyped, also by putting things in the same place improves manageability and extendability.
There was a problem hiding this comment.
I agree with you that enumerations will work better here. But what I don't like with this approach is to have one common enum for all cases. I better prefer to have a separateenumeration for every class. Otherwise it's not clear what notifications could come to a target doTask.
Anyway, after Qi's comment I removed the string parameter. You can check new version of the code.
There was a problem hiding this comment.
Thank you for your comments, anyway
orchagent/notifier.h
Outdated
| @@ -2,8 +2,8 @@ | |||
|
|
|||
| class Notifier : public Executor { | |||
| public: | |||
| Notifier(NotificationConsumer *select, Orch *orch) | |||
| : Executor(select, orch) | |||
| Notifier(NotificationConsumer *select, Orch *orch, const std::string& notifier_name) | |||
There was a problem hiding this comment.
notifier_name [](start = 74, length = 13)
I notice there is a similar name at NotificationConsumer.m_channel. I prefer using that one instread of a new parameter. #Closed
There was a problem hiding this comment.
Renamed to m_channel #Closed
There was a problem hiding this comment.
Let me clarify. I mean you could use getNotificationConsumer()->m_channel internally, instead ask user provide a new argument.
In reply to: 182863130 [](ancestors = 182863130)
What I did
I removed the notification handlers which were controlled by libsairedis and added direct support of notifications from ASIC_DB.
Why I did it
To remove global lock from orchagent.
How I verified it
Install into the switch and run it.
Also /var/log/syslog shows notifications from port status change.