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

sync: Add a protobuf for the existing sync protocol #1359

Merged
merged 12 commits into from
May 29, 2024

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 force-pushed the sync-proto branch 2 times, most recently from 2fc1475 to 819a9bd Compare May 28, 2024 02:21
@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.

Source/santasyncservice/syncv1.proto Outdated Show resolved Hide resolved
Source/santasyncservice/syncv1.proto Outdated Show resolved Hide resolved
@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.

Source/santasyncservice/SNTSyncEventUpload.mm Show resolved Hide resolved
Source/santasyncservice/SNTSyncEventUpload.mm Outdated Show resolved Hide resolved
Source/santasyncservice/SNTSyncPreflight.mm Outdated Show resolved Hide resolved
Source/santasyncservice/SNTSyncPreflight.mm Outdated Show resolved Hide resolved
tburgin
tburgin previously approved these changes May 28, 2024
@russellhancox russellhancox merged commit a23b67d into google:main May 29, 2024
12 checks passed
@russellhancox russellhancox deleted the sync-proto branch May 29, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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