Improve Peer Awareness's metadata to be updatable #181
Conversation
Yorkie should be able to update the client's metadata and notify other clients.
Codecov Report
@@ Coverage Diff @@
## main #181 +/- ##
==========================================
- Coverage 61.76% 61.05% -0.72%
==========================================
Files 38 38
Lines 3348 3423 +75
==========================================
+ Hits 2068 2090 +22
- Misses 1090 1143 +53
Partials 190 190
Continue to review full report at Codecov.
|
| // UpdateMetadata updates metadata of subscriber. | ||
| func (s *Subscription) UpdateMetadata(subscriber types.Client) { | ||
| s.subscriber.Metadata = subscriber.Metadata | ||
| } |
There was a problem hiding this comment.
Wouldn't it be necessary to use mutex locks for this part 🤔??
There was a problem hiding this comment.
It would be nice to write test cases that modify the metadata at the same time.
There was a problem hiding this comment.
@ppeeou @hackerwins Thank you for your comment!
This is the reason why the method was not locked.
To access Subscription, you can access it through PubSub.subscriptionsMap. And since subscriptionsMap is private, I thought that PubSub's mutex would be sufficient. This is because all PubSub methods use the same mutex. As a result, it was determined that it is safe to call other PubSub methods at the same time.
At the same time, when modifying the metadata, it was confirmed that the order in which the metadata is updated and the order in which it is published does not match. The UpdateMetadata API has been improved so that can be locked by the client ID. (c92ff31)
dc7303
force-pushed
the
feat-update-metadata
branch
2 times, most recently
from
523c851 to
c92ff31
Compare
The order of publishing after calling PubSub.UpdateSubscriber in UpdateMetadata should be synchronized. When requests from the same client are requested at the same time, the final result can be matched.
dc7303
force-pushed
the
feat-update-metadata
branch
from
c92ff31 to
6c8869d
Compare
Oh! My mistake. thank you for telling me :) |
|
@hackerwins @ppeeou |
|
When configuring the metadata without the logical clock, I am not sure that the problem will not occur when used to store the editor's cursor. I think it would be better to check and proceed further to see if the problem situation does not occur. |
|
I would like to do this work together after the distributed pubsub implementation. |
What this PR does / why we need it:
Added UpdateMetadata API to update the client's metadata.
Which issue(s) this PR fixes:
Fixes #153
Special notes for your reviewer:
If the added API design is ok, I will also work on
yorkie-js-sdk. I'll add aHow to use?section in the design document later.Does this PR introduce a user-facing change?:
Additional documentation:
Checklist: