-
Notifications
You must be signed in to change notification settings - Fork 264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NotificationProducer] add pipeline support #708
Changes from all commits
e6fa8cc
f00881e
f455362
c39b841
fa1ca24
102acb6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,14 @@ | ||
#include "notificationproducer.h" | ||
|
||
#define NON_BUFFERED_COMMAND_BUFFER_SIZE 1 | ||
|
||
swss::NotificationProducer::NotificationProducer(swss::DBConnector *db, const std::string &channel): | ||
m_db(db), m_channel(channel) | ||
m_ownedpipe(std::make_unique<swss::RedisPipeline>(db, NON_BUFFERED_COMMAND_BUFFER_SIZE)), m_pipe(m_ownedpipe.get()), m_channel(channel), m_buffered(false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @qiluo-msft The order of member initialization is defined by the standard and is well known. Member variables are initialized in the order they are defined in the class definition. If by any chance members are reordered in the definition any modern compiler will raise a warning (-Wreorder in gcc) and many of them should be treated as errors. Since we use -Wall + -Werror it is going to be catched. |
||
{ | ||
} | ||
|
||
swss::NotificationProducer::NotificationProducer(swss::RedisPipeline *pipeline, const std::string &channel, bool buffered): | ||
m_pipe(pipeline), m_channel(channel), m_buffered(buffered) | ||
{ | ||
} | ||
|
||
|
@@ -19,5 +26,19 @@ int64_t swss::NotificationProducer::send(const std::string &op, const std::strin | |
|
||
SWSS_LOG_DEBUG("channel %s, publish: %s", m_channel.c_str(), msg.c_str()); | ||
|
||
return m_db->publish(m_channel, msg); | ||
RedisCommand command; | ||
command.format("PUBLISH %s %s", m_channel.c_str(), msg.c_str()); | ||
|
||
if (m_buffered) | ||
{ | ||
m_pipe->push(command, REDIS_REPLY_INTEGER); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @qiluo-msft NotificationProducer has two invariants 1) Created from DBConnector and having it's own RedisPipeline in I see the same pattern beeing used for Table class (https://github.com/sonic-net/sonic-swss-common/blob/master/common/table.cpp#L38) and it was a motivation to follow the same desing. However, the table class uses raw pointer and manual memory allocation/deallocation + a flag to indicate wether we own the RedisPipeline object. So I came up with a bit simpler approach. |
||
|
||
// if operating in buffered mode return -1 as we can't know the number | ||
// of client's that have read the message immediately | ||
return -1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @qiluo-msft As stated by the comment on send() method the return code does not indicate the success or failure but the number of clients that read the message. In buffered mode we can't imidatelly tell how many clients read the message, so returning 1 might fool the user of this API that his consumer has already read message. |
||
} | ||
|
||
RedisReply reply = m_pipe->push(command); | ||
reply.checkReplyType(REDIS_REPLY_INTEGER); | ||
return reply.getReply<long long int>(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other places are using c++11. Is it a concern?
Like: pyext/py3/Makefile.am
And sairedis using c++11 depends on swss-common. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@qiluo-msft I don't see a problem. Everything builds successfully. I don't think other projects need to stay with c++11. c++14 has been around for a long time and adopted by compiler. As long as the headers are c++11 compatible and ABI does not change - everything works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a blocking issue?
If the change is really needed, I prefer changing the c++NN option in a consistent way, and change dependent libraries/applications first (swss->sairedis->swss-common). And change pyext/py[23]/Makefiles.am at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sairedis is c++14 - https://github.com/sonic-net/sonic-sairedis/blob/master/configure.ac#L121
swss is c++14 - https://github.com/sonic-net/sonic-swss/blob/master/configure.ac#L57
I believe not every app using swss-common is now c++14, not to mention app extensions.
My point is that swss-common may use c++-14 in .cpp but must have c++-11 compatible headers for those apps that might not be able to migrate yet. Python swss-common bindings compile a wrapper cpp based of the headers, that's why py[23]/Makefile.am aren't changed and stay c++-11.