Skip to content
This repository was archived by the owner on Feb 5, 2025. It is now read-only.

Conversation

@russellhancox
Copy link
Contributor

This PR is intended to have no impact on existing sync servers. The fields and enum values in the protobuf have been named such that their JSON equivalents match the existing constants we have in the codebase.

Adding this provides a few benefits:

  1. The protobuf serves as canonical documentation of the protocol in a form that's much easier to read than the existing code.
  2. Protobuf parsing of JSON is likely to be better than our hand-written version.
  3. We can (in a later PR) add a configuration option to use binary encoding instead of JSON, saving network during syncs.
  4. Servers written in other languages are easier to write and update as time goes on, especially as we extend the protocol.

@russellhancox russellhancox added the sync service Issues related to the sync service / protocol label May 28, 2024
@russellhancox russellhancox added this to the 2024.6 milestone May 28, 2024
@russellhancox russellhancox marked this pull request as ready for review May 28, 2024 02:59
@russellhancox russellhancox requested a review from a team as a code owner May 28, 2024 02:59
@tburgin
Copy link
Contributor

tburgin commented May 28, 2024

Can you git mv the .m -> .mm files so we can see the diff?

@tburgin
Copy link
Contributor

tburgin commented May 28, 2024

Oh all but SNTSyncEventUpload are diff'd, it would be good to get that one too.

@pmarkowsky
Copy link
Contributor

This is awesome. However given how strict proto3's JSON parsing can be.

We should be really careful to make sure we're backwards compatible.

This PR is intended to have no impact on existing sync servers. The fields and enum values in the protobuf have been named such that their JSON equivalents match the existing constants we have in the codebase.

Adding this provides a few benefits:

1. The protobuf serves as canonical documentation of the protocol in a form that's much easier to read than the existing code.
2. Protobuf parsing of JSON is likely to be better than our hand-written version.
3. We can (in a later PR) add a configuration option to use binary encoding instead of JSON, saving network during syncs.
4. Servers written in other languages are easier to write and update as time goes on, especially as we extend the protocol.
@russellhancox
Copy link
Contributor Author

Oh all but SNTSyncEventUpload are diff'd, it would be good to get that one too.

Unfortunately Git has no concept of a mv (the mv command is just shorthand for doing git add + git rm), it considers a file renamed if the name changes and a high percentage of the file is content is the same, which I've gone under in this one file. Before opening the PR I tried separating out the changes SNTSyncEventUpload from the rename but that didn't help either as it's still a single PR and GitHub is combining the diff results.

But I've just force-pushed the branch with 2 separate commits again as the "Files Changed" view still shows it as a new file if you view changes from all commits but you can view just the event upload commit on its own to see the smaller diff.

@russellhancox
Copy link
Contributor Author

This is awesome. However given how strict proto3's JSON parsing can be.

We should be really careful to make sure we're backwards compatible.

All the existing unit tests pass and I've tested a bunch with our internal sync server. Given how strict our hand-written parser was, this should work generally with other servers. We'll also get Moroz testing once this is merged and I'm planning to call this change out in the release notes as something to be very aware of.

tburgin
tburgin previously approved these changes May 28, 2024
@russellhancox russellhancox merged commit a23b67d into google:main May 29, 2024
@russellhancox russellhancox deleted the sync-proto branch May 29, 2024 18:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

sync service Issues related to the sync service / protocol

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants