-
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
🎨 Speeds up sending messages to user with socketio #5331
🎨 Speeds up sending messages to user with socketio #5331
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #5331 +/- ##
=========================================
- Coverage 87.3% 76.8% -10.5%
=========================================
Files 1321 542 -779
Lines 54151 27180 -26971
Branches 1172 202 -970
=========================================
- Hits 47282 20883 -26399
+ Misses 6619 6247 -372
+ Partials 250 50 -200
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.
Very nice.
Pleas make sure that if you start locally a service who's image is not present you are able to see the progress bar. After this is merged I will have a look at it's performance.
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.
Thank you! nice solution.
it's @sanderegg credit ;-) |
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.
thanks a lot for this!
@GitHK there is not test for that feature? I am not sure I can reproduce it. I propose to test in master and also create an automatic for it. Socketio plugin is really undertested! I can help with that. |
I will test it on was-master when you have it merged since I need to see the images being pulled. That's what causes a lot of messages to be delivered and the socktio delivery of messages to be slow. |
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.
Looks good to me. Thanks
|
What do these changes do?
This PR follows up from the incident reported by @matusdrobuliak66 and @GitHK on delays experienced consuming messages from socketio queue in rabbitMQ. @GitHK pointed out that the problem was in the delay caused by accessing redis everytime a socketio message was sent. @sanderegg proposed a solution that avoids this access by using instead the new rooms associated to
user_id
, i.e. rooms namedf"user:{user_id}"
.Related issue/s
How to test
Dev Checklist
DevOps Checklist