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

Replace the type of client_id and actor_id in protobuf #145

Merged

Conversation

autumn-n
Copy link
Contributor

What this PR does / why we need it:

A 24 bytes hex-encoded string can be replaced with a 12 bytes of array to reduce size of marshalled data with Protobuf.

Which issue(s) this PR fixes:

Fixes #36

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE, It just reduce the size of data between client and server.

Additional documentation:


Checklist:

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

A 24 bytes hex-encoded string can be replaced with a 12 bytes of array to reduce size of marshalled data with Protobuf.
@codecov
Copy link

codecov bot commented Jan 27, 2021

Codecov Report

Merging #145 (33b8c30) into main (8f26979) will increase coverage by 0.06%.
The diff coverage is 63.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #145      +/-   ##
==========================================
+ Coverage   56.99%   57.05%   +0.06%     
==========================================
  Files          33       33              
  Lines        3090     3106      +16     
==========================================
+ Hits         1761     1772      +11     
- Misses       1152     1154       +2     
- Partials      177      180       +3     
Impacted Files Coverage Δ
client/client.go 8.12% <0.00%> (ø)
pkg/document/time/actor_id.go 57.69% <60.00%> (+1.44%) ⬆️
yorkie/backend/db/db.go 66.66% <60.00%> (-33.34%) ⬇️
api/converter/from_pb.go 67.28% <100.00%> (ø)
api/converter/to_pb.go 85.53% <100.00%> (ø)
pkg/document/time/ticket.go 18.91% <100.00%> (+5.03%) ⬆️
yorkie/rpc/server.go 48.98% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f26979...747af9f. Read the comment docs.

@hackerwins
Copy link
Member

@autumn-n Thank you for sending PR. We'll review this soon.

Copy link
Member

@dc7303 dc7303 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 the wonderful work 👍
I left some amendments and questions. After checking, please be honest with your opinion.

Question

I think the modify the JS SDK also needs to be done to profile this PR. If you let me know if you're modifying the JS SDK, I think we'll see how we can help.
Are you modifying the JS SDK?

About test

How about adding a unit test for the added method?
I think it would be nice to add the normal case and exception cases of the method. This is because it will serve as a document and a safeguard for developers who analyze and modify this method in the future.

pkg/document/time/actor_id.go Outdated Show resolved Hide resolved
@autumn-n
Copy link
Contributor Author

@dc7303 Thanks for your comments

Question

I think the modify the JS SDK also needs to be done to profile this PR. If you let me know if you're modifying the JS SDK, I think we'll see how we can help.
Are you modifying the JS SDK?

Meanwhile, I'm already done. Once review has done, I will send a PR of JS SDK too.

About test

How about adding a unit test for the added method?
I think it would be nice to add the normal case and exception cases of the method. This is because it will serve as a document and a safeguard for developers who analyze and modify this method in the future.

Yes, you are right. I should have done that before. I'm gonna do it. In addition, It would be also great if there were more tests to figure out the intentions for new contributors.

@autumn-n autumn-n requested a review from dc7303 February 1, 2021 06:25
Copy link
Member

@dc7303 dc7303 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 😄

@dc7303 dc7303 removed their assignment Feb 1, 2021
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.

LGTM. Thanks for the contribution.

@hackerwins hackerwins merged commit ce9060c into yorkie-team:main Feb 1, 2021
@autumn-n autumn-n deleted the feature/replace-type-of-client-id branch February 2, 2021 01:32
autumn-n added a commit to autumn-n/yorkie-js-sdk that referenced this pull request Feb 2, 2021
hackerwins pushed a commit to yorkie-team/yorkie-js-sdk that referenced this pull request Feb 4, 2021
Replace the type of client_id to correspond with the Agent.
For more details: yorkie-team/yorkie#145
parkeunae pushed a commit to parkeunae/yorkie-js-sdk that referenced this pull request Jul 23, 2022
…ie-team#133)

Replace the type of client_id to correspond with the Agent.
For more details: yorkie-team/yorkie#145
jeonjonghyeok pushed a commit to jeonjonghyeok/yorkie that referenced this pull request Aug 4, 2022
…ie-team#145)

A 24 bytes hex-encoded string can be replaced with 12 bytes of the array to
reduce the size of marshaled data using Protobuf.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace ActorID type with UUID or Long instead of String to reduce the size of CRDT meta
3 participants