-
Notifications
You must be signed in to change notification settings - Fork 27
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
✨Adding rabbitmq based RPC for IPC between services #3909
✨Adding rabbitmq based RPC for IPC between services #3909
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3909 +/- ##
========================================
+ Coverage 85.0% 85.5% +0.5%
========================================
Files 934 781 -153
Lines 40180 34537 -5643
Branches 839 447 -392
========================================
- Hits 34175 29552 -4623
+ Misses 5784 4853 -931
+ Partials 221 132 -89
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Ok here are a few comments:
- why do we need yet another rabbitmq module ? can't we have an extension on the current one? (i.e. rabbitmq.py and now rabbitmq_robust_rpc.py?)
- in
rabbitmq.py
I made use of connection pooling, which is gone from your implementation. Also you had to re-implement all the setup/close mechanism that already exists there. - we will likely want to register a whole interface. is there any such mechanism?
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.
I added some comments.
I have some questions about this new approach but perhaps we can speak offline.
Thx.
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.
Very interesting! thanks for this. Please check my comments.
Did you try using that syntax?
await rpc.proxy.multiply(x=2, y=3) == 6
Code Climate has analyzed commit 5a79662 and detected 0 issues on this pull request. View more on Code Climate. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
What do these changes do?
Adds
RPC
to the backend stack to be used between different services inside the backend.Use cases:
director-v2
toagent
: ask to remove pending volumes and waits for the operation to be completeautoscaling
->agent
: ask if agent still needs to cleanup before putting the node to draindirector-v2
->dy-sidecar
(possibly): replaces http long running tasks polling with RPC pollingWhy this implementation and not others?
aio_pika
RPC helper has a very solid implementation. It was just thinly wrapped to deal with one edge case. For more details see tests.Bonus fixes:
dict
with{}
notationThus is required to fix issues
Related issue/s
How to test
Checklist