Publish events for select operation and getresponse failure #1142
Conversation
|
Request @kcudnik for review |
|
|
||
| using namespace sairedis; | ||
|
|
||
| event_handle_t g_events_handle; |
There was a problem hiding this comment.
If we decide use a global event publisher, then this should not be init/de-init in class level.
Because this is a global variable, but in dtor of RedisChannel the global variable will be deleted. Then when one RedisChannel instance deleted, all other instance will break when send event.
Please check the source code of deinit:
https://github.com/sonic-net/sonic-swss-common/blob/bcf48b26361f94e10a0eafc2c49c0bf0f440b2d5/common/events.cpp
Also if we decide use a global publisher, we need improve the event message, so receiver can identify which instance send the message.
And if every RedisChannel instance need have it's own event publisher, this should move to class member.
Because the thread safe issue I suggest we not use a global publisher, however it's depends on the requirement.
There was a problem hiding this comment.
According to offline sync, I suggest not necessary use global or class level event publisher, we can just use a publisher on stack to save resource, because we only need send failed event.
However even we using a publisher on stack, the thread safe issue still need fix, best solution is to add lock in events API.
There was a problem hiding this comment.
please dont use global objects
| @@ -32,6 +35,9 @@ RedisChannel::RedisChannel( | |||
| SWSS_LOG_NOTICE("creating notification thread"); | |||
|
|
|||
| m_notificationThread = std::make_shared<std::thread>(&RedisChannel::notificationThreadFunction, this); | |||
|
|
|||
| g_events_handle = events_init_publisher("sonic-events-swss"); | |||
There was a problem hiding this comment.
Channel will be use by multi-thread, however the implementation of events_init_publisher()/events_deinit_publisher() are no thread safe, because it access a std::map:
so suggest improve the events.cpp code first before this PR merge.
Or you can add lock here.
| { "operation_result", swss::Select::resultToString(result) }, | ||
| { "command", command }}; | ||
|
|
||
| event_publish(g_events_handle, "select-operation-failure", &m_event_params); |
There was a problem hiding this comment.
As my understand the requirement want to send this event to subscriptor, so e_event_params not necessary to be a data member.
Also UT should receive event send here and check, not necessary to check parameters.
And if we decide use a global publisher, zmq send is not thread safe, we need add lock here or improve the event code first:
https://github.com/sonic-net/sonic-swss-common/blob/bcf48b26361f94e10a0eafc2c49c0bf0f440b2d5/common/events.cpp#L150
|
|
||
| using namespace sairedis; | ||
|
|
||
| event_handle_t g_events_handle; |
There was a problem hiding this comment.
please dont use global objects
Why I did it
Add event_publish code to publish events when select operation fails or getresponse fails