-
Notifications
You must be signed in to change notification settings - Fork 6
Add basic implementation of YRoom
#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
Conversation
jupyter_rtc_core/rooms/yroom.py
Outdated
| return | ||
|
|
||
| # Broadcast the SyncUpdate to all other synced clients | ||
| self._broadcast_message_from(client_id, message, message_type="SyncUpdate") |
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.
broadcast messages to all clients could take a long time especially when there are many clients. It will make this call blocking and take a long time to finish and meanwhile, no other messages can be processed.
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 original implementation, there is a object stream buffer to hold those messages that need to be broadcasted and broadcasting in another task.
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 issue is that tornado.websocket.WebSocketHandler.write_message() is synchronous. We could make this async by running this logic in a separate thread (e.g. via ThreadPoolExecutor), but I don't know if that will improve performance due to the Python GIL.
I think the best way to address this is to: 1) time this using logs for now, then 2) address this performance issue if it proves to be a concern. What do you think?
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 original implementation used a object stream buffer to break up the operations in half and achieve async broadcasting leveraging a object stream/queue. We could definitely come back to this in future PRs.
|
Do we usually squash commits into one for each PR? Or we use that pattern. I am OK either way. |
jzhang20133
left a comment
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.
Approve to unblock and we can address issues in future PRs
|
@jzhang20133 Yup, generally we squash all PRs before merging them to keep the commit history clean. Also makes backporting changes easier, which is important post-1.0.0. |
|
More feedback from Jialin (that can be addressed in a future PR):
|
Description
Provides a basic implementation of
YRoom, as described in #3. TheYRoom:pycrdt.Doc()andpycrdt.Awareness()for a given file,SyncStep1,SyncUpdate, andAwarenessUpdatemessages usingpycrdt, andThis
YRoomdoes not do anything yet because it still requires:ContentsManager,YRoomManagerto initialize the file loader, andYRoomWebsocketthat calls methods onYRoomManagerto start rooms when clients connect to one without other collaborators.This PR also:
jupyter_server_fileidas a dependency,YjsClientGroup,YRoomManager, andYRoomLoader(name might change).YjsClientGroup.Follow-up issues
YRoomManagerYRoomWebsocket