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

Use document ID instead of key for watchDocuments API #491

Merged
merged 19 commits into from
Mar 17, 2023
Merged

Conversation

chacha912
Copy link
Contributor

@chacha912 chacha912 commented Mar 15, 2023

What this PR does / why we need it:

Address: #484

WatchDocuments API also needs to find documents by ID, not by key.

Which issue(s) this PR fixes:

Fixes #487
Related to #464

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


Additional documentation:


Checklist:

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

@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Merging #491 (37a1151) into main (bfdbabf) will decrease coverage by 0.12%.
The diff coverage is 11.29%.

@@            Coverage Diff             @@
##             main     #491      +/-   ##
==========================================
- Coverage   48.79%   48.68%   -0.12%     
==========================================
  Files          69       69              
  Lines        5962     5953       -9     
==========================================
- Hits         2909     2898      -11     
- Misses       2712     2714       +2     
  Partials      341      341              
Impacted Files Coverage Δ
api/converter/from_pb.go 53.65% <0.00%> (-1.00%) ⬇️
pkg/document/key/key.go 100.00% <ø> (ø)
server/backend/sync/etcd/pubsub.go 0.00% <0.00%> (ø)
server/rpc/yorkie_server.go 44.80% <0.00%> (-1.49%) ⬇️
client/client.go 10.28% <2.08%> (-0.19%) ⬇️
server/backend/sync/memory/pubsub.go 41.11% <32.43%> (-6.01%) ⬇️
api/converter/to_pb.go 63.87% <50.00%> (+0.27%) ⬆️
server/backend/sync/memory/coordinator.go 30.00% <55.55%> (+30.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@hackerwins hackerwins marked this pull request as ready for review March 17, 2023 07:11
@hackerwins hackerwins self-requested a review March 17, 2023 08:18
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.

@hackerwins hackerwins merged commit 67770d3 into main Mar 17, 2023
@hackerwins hackerwins deleted the watch-doc-id branch March 17, 2023 08:26
hackerwins pushed a commit that referenced this pull request Mar 17, 2023
1. Change to use ID instead of Key in WatchDocument
  - By introducing RemoveDocument API, multiple IDs can be mapped to a
    single key(multiple removed IDs and one active ID)
2. Change WatchDocuments API to WatchDocument API
  - In the cluster mode conversion task, one document is mapped to one
    server, and single document mapping is a simplified cluster
  - This can cause Changing the structure where the client maintains
    one WatchDocuments stream to maintain multiple streams.
3. Remove unnecessary types due to the change in step 2
  - The implementation was simplified by changing the structure in
    step 2, so unnecessary data types were cleaned up.

Co-authored-by: hackerwins <hackerwins@gmail.com>
@hackerwins hackerwins added the protocol changed 📝 Whether the protocol has changed label Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
protocol changed 📝 Whether the protocol has changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify the DocumentKeys field in the DocEvent to pass a single value of DocumentKey instead of an array
2 participants