Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[dash] Improve dash orchagent ZMQ code. #2836

Merged
merged 11 commits into from
Jul 6, 2023
Merged

[dash] Improve dash orchagent ZMQ code. #2836

merged 11 commits into from
Jul 6, 2023

Conversation

liuh-80
Copy link
Contributor

@liuh-80 liuh-80 commented Jun 25, 2023 •
edited
Loading

Improve dash orchagent code.

What I did
Move ZmqOrch related code to new files.
Remove unused code.
Change ZmqOrch::doTask() to use ConsumerBase as parameter.
Fix orchagent start parameter parse bug.
Fix dash orch can't receive data when ZMQ disabled issue.

Why I did it
Improve dash orchagent code.
HLD: https://github.com/sonic-net/SONiC/blob/master/doc/sonic-swss-common/ZMQ%20producer-consumer%20state%20table%20design.md?plain=1

How I verified it
Pass all UT.

Details if related

@liuh-80 liuh-80 mentioned this pull request Jun 25, 2023
Merged
mint570
mint570 reviewed Jun 26, 2023
View reviewed changes
orchagent/dash/dashorchbase.h Outdated
void drain() override;
};

class DashOrchBase : public Orch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be named as "ZmqOrchBase" for other non-dash orchs to use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this class is only designed for dash orch. because only dash orch using ZMQ to receive data from GNMI.
The reason I rename this from ZmqOrch to DashOrchBase because there are comments that create consumer/zmqconsumer based on parameter is wried for ZmqOrch, because the behavior is only required by dash so I rename this class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are looking at this to find potential performance improvement in our use case as well.
Wouldn't it be nice to make it usable to other orchs (and eventually migrate other orchs to use zmq)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think eventually we will support other orch.
Migrate ZMQ also need client side change, currently the Dash orch only have gnmi as client. so fir the first step we plan only for Dash orch.
And if other Orch want ZMQ support, we can do it in another PR with more improvement, for example create a consumer builder interface for ZmqConsumer and redis based Consumer, and change all Orch ctor code to accept a build as parameter.
However that's need change all orch code, so it's better to put in a seprate PR.

@liuh-80 liuh-80 marked this pull request as ready for review June 27, 2023 01:16
@liuh-80 liuh-80 requested a review from prsunny as a code owner June 27, 2023 01:16
@liuh-80 liuh-80 requested a review from qiluo-msft June 27, 2023 01:16
@liuh-80 liuh-80 mentioned this pull request Jun 28, 2023
Merged
qiluo-msft
qiluo-msft reviewed Jun 29, 2023
View reviewed changes
orchagent/dash/dashorchbase.h Outdated
void drain() override;
};

class DashOrchBase : public Orch
Copy link
Contributor

@qiluo-msft qiluo-msft Jun 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DashOrchBase

Can we move renaming into another separate PR? Seems like Dash is not an accurate name, because it is not bound to dash project. Waiting community to provide more feedback on the name.

Other changes are easy to review and merge. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, rename back to ZmqOrch

@prsunny
Copy link
Collaborator

prsunny commented Jun 30, 2023

@liuh-80 , can you please fix the description of PR to better align with new code? Also, "why" section. Link to any HLD if there is one.

@liuh-80
Copy link
Contributor Author

liuh-80 commented Jul 3, 2023

@liuh-80 , can you please fix the description of PR to better align with new code? Also, "why" section. Link to any HLD if there is one.

Fixed, Description updated also add HLD link.

@liuh-80 liuh-80 requested a review from Pterosaur July 5, 2023 08:24
qiluo-msft
qiluo-msft reviewed Jul 5, 2023
View reviewed changes
orchagent/zmqorch.cpp Outdated
void ZmqConsumer::drain()
{
if (!m_toSync.empty())
(static_cast<ZmqOrch*>(m_orch))->doTask(static_cast<ZmqConsumer&>(*this));
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*this is already in this class ZmqConsumer. Is this casting really needed?
#Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

qiluo-msft
qiluo-msft reviewed Jul 5, 2023
View reviewed changes
orchagent/zmqorch.cpp Outdated
void ZmqOrch::doTask(Consumer &consumer)
{
// When ZMQ disabled, forward data from Consumer
doTask(static_cast<ConsumerBase &>(consumer));
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConsumerBase

Prefer traditional casting for upcasting for readability. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@liuh-80 liuh-80 merged commit 982a341 into sonic-net:dash Jul 6, 2023
11 checks passed
@liuh-80 liuh-80 deleted the dev/liuh/improve-code branch July 6, 2023 11:59
mint570
mint570 reviewed Jul 12, 2023
View reviewed changes
orchagent/zmqorch.h
#include <orch.h>
#include "zmqserver.h"

class ZmqConsumer : public ConsumerBase {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not have all the consumer methods. How does warm boot work?
Looks like the current codes just cast the executor to be Consumer in the warmboot logic. It won't work for ZmqConsumer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that currently the Dash no plan for warm-reboot support.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two PRs:
#2857
sonic-net/sonic-swss-common#803

Could you take a look at them please?

@mint570 mint570 mentioned this pull request Jul 14, 2023
Closed
theasianpianist pushed a commit to theasianpianist/sonic-swss that referenced this pull request Jul 21, 2023
@liuh-80 @theasianpianist
[dash] Improve dash orchagent ZMQ code. (sonic-net#2836)
df04802 
Improve dash orchagent code.

**What I did**
Move ZmqOrch related code to new files.
Remove unused code.
Change ZmqOrch::doTask() to use ConsumerBase as parameter.
Fix orchagent start parameter parse bug.
Fix dash orch can't receive data when ZMQ disabled issue.

**Why I did it**
Improve dash orchagent code.
HLD: https://github.com/sonic-net/SONiC/blob/master/doc/sonic-swss-common/ZMQ%20producer-consumer%20state%20table%20design.md?plain=1

**How I verified it**
Pass all UT.

**Details if related**
theasianpianist pushed a commit to theasianpianist/sonic-swss that referenced this pull request Jul 25, 2023
@liuh-80 @theasianpianist
[dash] Improve dash orchagent ZMQ code. (sonic-net#2836)
4872d1c 
Improve dash orchagent code.

**What I did**
Move ZmqOrch related code to new files.
Remove unused code.
Change ZmqOrch::doTask() to use ConsumerBase as parameter.
Fix orchagent start parameter parse bug.
Fix dash orch can't receive data when ZMQ disabled issue.

**Why I did it**
Improve dash orchagent code.
HLD: https://github.com/sonic-net/SONiC/blob/master/doc/sonic-swss-common/ZMQ%20producer-consumer%20state%20table%20design.md?plain=1

**How I verified it**
Pass all UT.

**Details if related**
theasianpianist pushed a commit to theasianpianist/sonic-swss that referenced this pull request Jul 25, 2023
@liuh-80 @theasianpianist
[dash] Improve dash orchagent ZMQ code. (sonic-net#2836)
6f008a6 
Improve dash orchagent code.

**What I did**
Move ZmqOrch related code to new files.
Remove unused code.
Change ZmqOrch::doTask() to use ConsumerBase as parameter.
Fix orchagent start parameter parse bug.
Fix dash orch can't receive data when ZMQ disabled issue.

**Why I did it**
Improve dash orchagent code.
HLD: https://github.com/sonic-net/SONiC/blob/master/doc/sonic-swss-common/ZMQ%20producer-consumer%20state%20table%20design.md?plain=1

**How I verified it**
Pass all UT.

**Details if related**
theasianpianist pushed a commit to theasianpianist/sonic-swss that referenced this pull request Jul 25, 2023
@liuh-80 @theasianpianist
[dash] Improve dash orchagent ZMQ code. (sonic-net#2836)
25c69c9 
Improve dash orchagent code.

**What I did**
Move ZmqOrch related code to new files.
Remove unused code.
Change ZmqOrch::doTask() to use ConsumerBase as parameter.
Fix orchagent start parameter parse bug.
Fix dash orch can't receive data when ZMQ disabled issue.

**Why I did it**
Improve dash orchagent code.
HLD: https://github.com/sonic-net/SONiC/blob/master/doc/sonic-swss-common/ZMQ%20producer-consumer%20state%20table%20design.md?plain=1

**How I verified it**
Pass all UT.

**Details if related**
theasianpianist pushed a commit to theasianpianist/sonic-swss that referenced this pull request Jul 25, 2023
@liuh-80 @theasianpianist
[dash] Improve dash orchagent ZMQ code. (sonic-net#2836)
ee4f7b3 
Improve dash orchagent code.

**What I did**
Move ZmqOrch related code to new files.
Remove unused code.
Change ZmqOrch::doTask() to use ConsumerBase as parameter.
Fix orchagent start parameter parse bug.
Fix dash orch can't receive data when ZMQ disabled issue.

**Why I did it**
Improve dash orchagent code.
HLD: https://github.com/sonic-net/SONiC/blob/master/doc/sonic-swss-common/ZMQ%20producer-consumer%20state%20table%20design.md?plain=1

**How I verified it**
Pass all UT.

**Details if related**
theasianpianist pushed a commit to theasianpianist/sonic-swss that referenced this pull request Jul 27, 2023
@liuh-80 @theasianpianist
(conflict)[dash] Improve dash orchagent ZMQ code. (sonic-net#2836)
ff873f5 
Improve dash orchagent code.

**What I did**
Move ZmqOrch related code to new files.
Remove unused code.
Change ZmqOrch::doTask() to use ConsumerBase as parameter.
Fix orchagent start parameter parse bug.
Fix dash orch can't receive data when ZMQ disabled issue.

**Why I did it**
Improve dash orchagent code.
HLD: https://github.com/sonic-net/SONiC/blob/master/doc/sonic-swss-common/ZMQ%20producer-consumer%20state%20table%20design.md?plain=1

**How I verified it**
Pass all UT.

**Details if related**
theasianpianist pushed a commit to theasianpianist/sonic-swss that referenced this pull request Jul 27, 2023
@liuh-80 @theasianpianist
(conflict)[dash] Improve dash orchagent ZMQ code. (sonic-net#2836)
b2c25dc 
Improve dash orchagent code.

**What I did**
Move ZmqOrch related code to new files.
Remove unused code.
Change ZmqOrch::doTask() to use ConsumerBase as parameter.
Fix orchagent start parameter parse bug.
Fix dash orch can't receive data when ZMQ disabled issue.

**Why I did it**
Improve dash orchagent code.
HLD: https://github.com/sonic-net/SONiC/blob/master/doc/sonic-swss-common/ZMQ%20producer-consumer%20state%20table%20design.md?plain=1

**How I verified it**
Pass all UT.

**Details if related**
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pterosaur Pterosaur Pterosaur left review comments

@mint570 mint570 mint570 left review comments

@qiluo-msft qiluo-msft qiluo-msft approved these changes

@prsunny prsunny Awaiting requested review from prsunny prsunny is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@liuh-80 @prsunny @qiluo-msft @Pterosaur @mint570