-
Notifications
You must be signed in to change notification settings - Fork 33
Split ReplicationChannel
into ServerChannel
and ClientChannel
#490
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #490 +/- ##
==========================================
+ Coverage 81.18% 81.70% +0.52%
==========================================
Files 52 52
Lines 2992 2996 +4
==========================================
+ Hits 2429 2448 +19
+ Misses 563 548 -15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
For clarity. While doing so, I noticed that we register an extra client channel. While the server requires two channels, the client only needs one. I also removed wrong comment about receive functions. They aren't called by backends.
/// | ||
/// <div class="warning"> | ||
/// | ||
/// Should only be called from the messaging backend. |
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.
Why remove this?
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.
It's not called by messaging backends. It's called by Replicon to read the messages. Maybe I should even make it private 🤔
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.
Yeah make it private if it's not for the public or backend APIs.
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.
Done!
/// | ||
/// <div class="warning"> | ||
/// | ||
/// Should only be called from the messaging backend. |
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.
And this
I had to reorganize test since integration tests don't have access to private methods.
For clarity. While doing so, I noticed that we register an extra client channel. While the server requires two channels, the client only needs one, the client only needs one for the acks.
Based on #488 to avoid too many conflicts.