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

feat: collab entities in protobuf format #226

Merged
merged 13 commits into from
Jul 24, 2024
Merged

Conversation

khorshuheng
Copy link
Contributor

@khorshuheng khorshuheng commented Jul 5, 2024

Currently, we are using bincode to serialize and deserialize Rust struct that needs to be transferred over the network. In order to achieve better backward compatibility and manage schema changes, we will be migrating to protobuf for serialization.

We will start with this PR, that creates the protobuf requires for the serialization/deserialization of these Rust struct:

Comments have been added to the protobuf based on my current understanding, which might not be accurate. Hence, review is needed to make sure that the comments are not misleading.

We will follow up with subsequent PRs that update the serialization and deserialization of the above structs.

@CLAassistant
Copy link

CLAassistant commented Jul 5, 2024

CLA assistant check
All committers have signed the CLA.

@khorshuheng khorshuheng force-pushed the collab-entity-protobuf branch 5 times, most recently from 4f84865 to d32697c Compare July 5, 2024 08:01
@Horusiath Horusiath self-requested a review July 5, 2024 08:05
@khorshuheng khorshuheng force-pushed the collab-entity-protobuf branch 3 times, most recently from de835d7 to 4df4072 Compare July 5, 2024 08:15
@khorshuheng
Copy link
Contributor Author

khorshuheng commented Jul 5, 2024

The test failure is likely due to the regex_automata downgrade:

source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "8263075bb86c5a1b1427b5ae862e8889656f126e9f77c484496e8b47cf5c5558"
dependencies = [
 "regex-automata",
 "regex-automata 0.1.10",
]

Going to find out how to resolve this. This is due to introduction of the prost-build build dependencies.

@khorshuheng khorshuheng force-pushed the collab-entity-protobuf branch 2 times, most recently from 464274d to 6b16b39 Compare July 5, 2024 12:23
@khorshuheng
Copy link
Contributor Author

The test failure is likely due to the regex_automata downgrade:

source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "8263075bb86c5a1b1427b5ae862e8889656f126e9f77c484496e8b47cf5c5558"
dependencies = [
 "regex-automata",
 "regex-automata 0.1.10",
]

Going to find out how to resolve this. This is due to introduction of the prost-build build dependencies.

Updated tracing-subscriber version due to: tokio-rs/tracing#2573

@khorshuheng
Copy link
Contributor Author

Added protobuf definition for CreateCollabParams and BatchCreateCollabParams.

@khorshuheng
Copy link
Contributor Author

Just realized that we could potentially simplified this via: https://github.com/tokio-rs/prost?tab=readme-ov-file#tag-inference-for-existing-types . Exploring this now.

@khorshuheng
Copy link
Contributor Author

CollabSnapshot is being called in get_last_snapshot etc, but those methods are not currently in use anywhere in AppFlowy. In other word, although we are serializing this struct, it is never deserialized back again. Is this intended? @appflowy

@khorshuheng khorshuheng force-pushed the collab-entity-protobuf branch from c317305 to 7e88387 Compare July 10, 2024 03:17
@khorshuheng
Copy link
Contributor Author

CollabSnapshot is being called in get_last_snapshot etc, but those methods are not currently in use anywhere in AppFlowy. In other word, although we are serializing this struct, it is never deserialized back again. Is this intended? @appflowy

Had a discussion on this and confirmed that CollabSnapshot will be deprecated.

@khorshuheng
Copy link
Contributor Author

@Horusiath @appflowy Any suggestion as to whether the generated code should be checked in? Right now we have both cases in different libraries and repositories across the org.

@Horusiath
Copy link
Collaborator

Horusiath commented Jul 11, 2024

@khorshuheng personally I'm not a fan of checking in the auto-generated code - it obscures the PR review - but it should be part of the build step.

@appflowy
Copy link
Contributor

It needs to test it can be integrate with frontend application. Since it involves build.rs

@khorshuheng
Copy link
Contributor Author

It needs to test it can be integrate with frontend application. Since it involves build.rs

I will test this via AppFlowy-IO/AppFlowy#5745

@khorshuheng khorshuheng force-pushed the collab-entity-protobuf branch 3 times, most recently from 8ce39d4 to aa6be8b Compare July 22, 2024 05:50
@khorshuheng khorshuheng force-pushed the collab-entity-protobuf branch from aa6be8b to d40b450 Compare July 22, 2024 06:35
@khorshuheng khorshuheng requested a review from appflowy July 24, 2024 02:42
@appflowy appflowy merged commit a6fe08c into main Jul 24, 2024
7 checks passed
@appflowy appflowy deleted the collab-entity-protobuf branch July 24, 2024 03:30
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.

4 participants