-
Notifications
You must be signed in to change notification settings - Fork 17
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
RTC-512 Add Room.State module and peer disconnected timeout #178
Conversation
5e41254
to
c318a8d
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #178 +/- ##
==========================================
+ Coverage 86.88% 87.24% +0.36%
==========================================
Files 73 74 +1
Lines 1433 1482 +49
==========================================
+ Hits 1245 1293 +48
- Misses 188 189 +1
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
98519d6
to
e006de3
Compare
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 find the peerlessPurgeTimeout
and peerDisconnectedTimeout
a bit simmilar. How about renaming the peerlessPurgeTimeout
to roomIdleTimeout
?
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.
The room doesn't have to be idle for the purge to happen, though -- there can be components present...
lib/jellyfish/peer.ex
Outdated
@@ -32,7 +39,8 @@ defmodule Jellyfish.Peer do | |||
socket_pid: pid() | nil, | |||
engine_endpoint: Membrane.ChildrenSpec.child_definition(), | |||
tracks: %{Track.id() => Track.t()}, | |||
metadata: any() | |||
metadata: any(), | |||
last_time_connected: non_neg_integer() | nil |
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.
[nitpick] monotonic_time
returns integer()
lib/jellyfish/room.ex
Outdated
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'm really unsure about the changes introduced here. I see why we're choosing to extract the state to a separate module, but as it stands, the implementation is "uneven", the abstraction level fluctuates -- e.g. notifications are sometimes emitted by the functions in state, sometimes by the functions here; calls to State can update a single key in the struct, or do a lot of additional logic as well.
IMO the State module should mainly be considered with, well, keeping track of the room state -- any and all additional tasks, such as emitting telemetry events and notifications, should be performed by Room. However, ensuring this distinction in the code could potentially diminish the benefits we achieved from splitting the state in the first place...
Discussion is welcome
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 agree that the abstraction level fluctuates.I thought about how to better handle situations like broadcasting events, but it is pretty hard because many events require a lot of information e.g :track_removed
event the API of the remove_track
would look terryifing to return all needed information. As I see currently I could move the last two events from Room to State, but I am not sure if this will help a lot.
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.
In my opinion it's a good thing that we moved some of the logic to the State
.
It is true however that it is sometimes unclear what actions take place in the State
and which withing the Room
.
I think we can clear this up a bit.
First, we can move sending all Event
s to State
.
Maybe it would be nice to send them from Room
. But since all of them but two are emitted from State
we can just make it a convention.
We can also get rid of any changes to state
struct within the Room
and move it to the State
.
4f2d3c6
to
55e364d
Compare
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.
LGTM 🎸
Co-authored-by: Przemysław Rożnawski <[email protected]>
55e364d
to
ecba4d5
Compare
This PR introduces two changes:
Acknowledging the stipulations set forth:
PR to docs
PR to elixir SDK
PR to Python SDK