Skip to content
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

Introduce Broadcast API #631

Merged
merged 25 commits into from
Oct 2, 2023
Merged

Introduce Broadcast API #631

merged 25 commits into from
Oct 2, 2023

Conversation

sejongk
Copy link
Contributor

@sejongk sejongk commented Aug 28, 2023

What this PR does / why we need it:
This pull request introduces the new "broadcast" feature, allowing for the sharing of general events.

  • Add Document.Broadcast API
    • Update protocol: Add broadcast-related types
    • SDK: Implement Document.broadcast(event)
    • Server: Implement Broadcast API
  • SDK: Implement Document.Subscribe

Which issue(s) this PR fixes:

Address #628

Special notes for your reviewer:
The following are the current implementations that need to be discussed.
I added test cases for the below specs.

  • Clients who don't subscribe to an event type can broadcast for that event type.
  • The payload in a broadcast event should be serializable to JSON.
  • Clients can keep watching the document although an error occurs during the handling of a broadcast event.

Does this PR introduce a user-facing change?:


Additional documentation:


Checklist:

  • Added relevant tests or not required
  • Didn't break anything

@sejongk sejongk marked this pull request as draft August 28, 2023 09:51
@sejongk sejongk self-assigned this Aug 28, 2023
@sejongk sejongk added the enhancement 🌟 New feature or request label Aug 28, 2023
@sejongk sejongk added the protocol changed 📝 Whether the protocol has changed label Sep 6, 2023
@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

Attention: 165 lines in your changes are missing coverage. Please review.

Comparison is base (940941a) 49.47% compared to head (bde74a4) 48.86%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #631      +/-   ##
==========================================
- Coverage   49.47%   48.86%   -0.61%     
==========================================
  Files          69       69              
  Lines        9951    10082     +131     
==========================================
+ Hits         4923     4927       +4     
- Misses       4512     4635     +123     
- Partials      516      520       +4     
Files Coverage Δ
api/types/auth_webhook.go 34.00% <100.00%> (+1.34%) ⬆️
pkg/document/time/actor_id.go 55.76% <ø> (ø)
api/converter/from_pb.go 41.34% <0.00%> (-0.75%) ⬇️
api/converter/to_pb.go 52.48% <0.00%> (-0.27%) ⬇️
server/backend/sync/memory/coordinator.go 31.25% <0.00%> (-1.01%) ⬇️
pkg/document/document.go 38.12% <25.92%> (-2.91%) ⬇️
server/rpc/yorkie_server.go 44.98% <0.00%> (-5.54%) ⬇️
client/client.go 12.35% <0.00%> (-1.23%) ⬇️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sejongk sejongk marked this pull request as ready for review September 6, 2023 03:01
@sejongk
Copy link
Contributor Author

sejongk commented Sep 13, 2023

@hackerwins
There are some points to note regarding this implementation:

  1. DocumentBroadcast is one of the DocEvent types, meaning that broadcast events are delivered as DocEvent objects through the watch stream.
  2. Given the frequency of broadcasts and the complexity of the implementation, it appears to be a good practice to always send broadcasts to all clients, allowing clients to filter them out locally. If necessary, we can consider implementing server-side event filtering at a later stage.
  3. The PubSub module can remain mostly unchanged since we won't be implementing server-side event filtering as mentioned above.

Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution.

It would be great to refine SubscribeBroadcastEvent along with integrating the Attach and Watch interfaces(#584).

@hackerwins hackerwins merged commit 7c3bc6f into main Oct 2, 2023
2 checks passed
@hackerwins hackerwins deleted the add-broadcast branch October 2, 2023 11:56
@hackerwins hackerwins added the documentation 📔 Improvements or additions to documentation label Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📔 Improvements or additions to documentation enhancement 🌟 New feature or request protocol changed 📝 Whether the protocol has changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants