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

Messaging consolidation #14

Merged
merged 10 commits into from
Jul 7, 2020
Merged

Messaging consolidation #14

merged 10 commits into from
Jul 7, 2020

Conversation

rtobar
Copy link
Contributor

@rtobar rtobar commented Jul 7, 2020

This set of changes contains a series of improvements on the inter-node messaging system:

  • It creates a single Context object used by both the ZeroMQ publisher/subscriber and the ZeroRPC client/server code of the Node Manager. Previously the ZeroMQ publisher/subscriber, the ZeroRPC server, and each ZeroRPC client created their own Context objects, which implicitly creates a new I/O thread. Reducing these Context objects to a single one reduces this to a single I/O thread shared by all ZeroMQ-based code.
  • The naming of most of the internal members of the ZeroMQ publisher/subscriber class has been improved for easier code readibility.
  • A docstring has been added to the same class to explain how the code works internally, and which other alternatives have been sought.
  • The subscription mechanism has been improved to ensure the physical connection between a subscriber and the publisher it attempts to connect to is actually established before claiming success. This otherwise race condition was previously a constant source of (very) spurious errors in unit tests, and while it cannot be proven that the race condition doesn't exist anymore, we hope this change will improve the situation.

rtobar added 10 commits July 6, 2020 16:57
@rtobar
Remove unused import
85da574 
Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@rtobar
Remove support for alternative RPC frameworks
382e1ba 
It has been years since we use exclusively ZeroRPC for our RPC needs, so
there really is no point on maintaining the alternatives. If we need
them at some point we can bring them back, but most probably we will do
fine without them.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@rtobar
Explicitly close ZeroMQ sockets
18bca68 
We previously didn't do this, and while it didn't give us any problems
it wasn't really what a good citizen should do with their sockets.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@rtobar
Perform hostname resolution in caller thread
6e2e309 
This change should decrease the latency with which a subscriber socket
connects to its publisher, hopefully making the system more stable.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@rtobar
Create a single ZeroMQ Context per Node Manager
d3e85a3 
This commit introduces the creation of a unique ZeroMQ Context instance
at the level of the NodeManager class that can be used instead of the
per-class Context instances we have been using in the ZeroPubSubMixIn,
ZeroRPCClient and ZeroRPCServer classes. In order to be usable from all
of these, this context object must be created using zerorpc's Context
class, which contains a few additions on top of a ZeroMQ's Context, and
that are used by ZeroRPC's classes.

Instead of managing their own Context instances, now the ZMQPubSubMixIn,
the ZeroRPCClient and the ZeroRPCServer classes all share the new,
unique Context instance created by the NodeManager classes, who takes
care of creating it and terminating it.

This change not only makes the code easier to maintain (because there is
less resource management in general), it also should reduce the number
of threads used by the node manager, since each Context will create at
least one I/O thread.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@rtobar
Import gevent and zerorpc unconditionally
6333981 
We previously imported these modules only when required, since the RPC
framework to use was a runtime choice. Now that we have removed that
choice, and zerorpc is the only alternative, there is no point anymore
on having conditional imports spread across the rpc module.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@rtobar
Unify code for background thread creation
408b15d 
This is simply to keep the code more maintainable and easier to follow.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@rtobar
Simplify member variable names for easier reading
6966645 
We already are within the context of an ZMQPubSubMixIn class so there's
no need for additional "zmq" prefixes/suffixes in member names, it makes
the code a bit more difficult to follow otherwise.

The names of the different members also didn't properly describe what
they did. The new names instead reflect a bit better the intent behind
them. There was also a log statement that read "Receiving" instead of
"Publishing", as it most probably got lost in this confusion of names.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@rtobar
Add design overview docs for ZMQPubSubMixIn
9db41c3 
This should help future readers (including myself) understand what's
going on and why we chose to go this way in the first place.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@rtobar
Monitor socket for connection establishment
67b58ac 
Only once the connection has been fully made we can declare success and
let the flow continue in the thread that initiated the subscription.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@rtobar rtobar requested a review from mdolensk July 7, 2020 05:57
@coveralls
Copy link

coveralls commented Jul 7, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 76.642% when pulling 67b58ac on messaging-consolidation-take2 into c86850e on master.

@mdolensk
Copy link

mdolensk commented Jul 7, 2020

Thanks for a substantial improvement in code maintainability (removal of dead code, new comments, naming, single 0MQ context object ...)

@mdolensk mdolensk closed this Jul 7, 2020
@mdolensk
Copy link

mdolensk commented Jul 7, 2020

YAN-322 inserting ticket number

@rtobar rtobar reopened this Jul 7, 2020
@rtobar
Copy link
Contributor Author

rtobar commented Jul 7, 2020

@mdolensk Thanks for reviewing the PR. Would you mind marking it as reviewed, but without closing it? I'll then merge the code into the master branch. That way there is a better record of the acceptance actually happening.

mdolensk
mdolensk approved these changes Jul 7, 2020
View reviewed changes
Copy link

@mdolensk mdolensk left a comment

Choose a reason for hiding this comment

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

Thanks for a substantial improvement in code maintainability (removal of dead code, new comments, naming, single 0MQ context object ...)

@rtobar rtobar merged commit 67b58ac into master Jul 7, 2020
@rtobar rtobar deleted the messaging-consolidation-take2 branch July 7, 2020 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdolensk mdolensk mdolensk approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@rtobar @coveralls @mdolensk