Page MenuHomePhabricator
Differential D4342

[DRAFT] [services] Backup/Blob - Use less oneof
AbandonedPublic
Actions

Authored by michal on Jun 23 2022, 7:16 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Sep 27, 4:07 PM
Unknown Object (File)
Fri, Sep 27, 4:06 PM
Unknown Object (File)
Fri, Sep 27, 3:58 PM
Unknown Object (File)
Thu, Sep 26, 11:00 PM
Unknown Object (File)
Wed, Sep 25, 3:07 AM
Unknown Object (File)
Aug 27 2024, 4:07 PM
Unknown Object (File)
Aug 27 2024, 1:41 PM
Unknown Object (File)
Aug 17 2024, 4:51 PM
View All Files
Subscribers
• abosh
• adrian
ashoat
atul
tomek

Details

Reviewers
tomek
ashoat
• karol
Summary

Linear task: https://linear.app/comm/issue/ENG-1052/reconsider-using-oneof-in-services

This is just a draft with a proposition on where we can use less of oneof. Once this gets accepted, I'm going to start implementing the changes in the services' logic.

Test Plan

None, this is a design discussion.

Diff Detail

Repository
rCOMM Comm
Branch
oneof
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• karol created this revision.Jun 23 2022, 7:16 AM
Herald added subscribers: • abosh, atul, • adrian and 2 others. · View Herald TranscriptJun 23 2022, 7:16 AM
Harbormaster returned this revision to the author for changes because remote builds failed.Jun 23 2022, 7:17 AM
Harbormaster failed remote builds in B9918: Diff 13712!
• karol requested review of this revision.Jun 23 2022, 7:18 AM
• karol edited the summary of this revision. (Show Details)
• karol edited the test plan for this revision. (Show Details)
• karol added reviewers: tomek, ashoat.

Of course, the services build failed, because these changes are not considered in the code.

tomek requested changes to this revision.Jun 27 2022, 5:35 AM
tomek added 1 blocking reviewer(s): ashoat.
tomek added inline comments.
native/cpp/CommonCpp/grpc/protos/backup.proto
32–38

Do we, after this change, always need to include userID, deviceID and keyEntropy in CreateNewBackupRequest? Is this really necessary?

This revision now requires changes to proceed.Jun 27 2022, 5:35 AM
• karol mentioned this in D4245: [services] Backup - Add current id and attachment holders.Jun 28 2022, 4:01 AM
ashoat requested changes to this revision.Jun 28 2022, 12:55 PM
  1. We should avoid wasted data transfer, eg. repeating the same information multiple times. This is why we initially started using oneof, and what @palys-swm brought up above. We want to move away from oneof while keeping this property
  2. We should avoid forcing multiple round trips when we can communicate the same info in one trip. This is the original feedback about oneof, and what we're trying to improve in ENG-1052

The way to think about this is basically... how many round trips do we need in total? For each trip, we should have a message that has the info for that trip. And then we can use oneof for the different types of messages.

In other words: we should avoid using oneof when it increases the number of round trips.

michal commandeered this revision.Feb 16 2024, 2:39 AM
michal abandoned this revision.
michal added a reviewer: • karol.

Old diff, that isn't applicable to the current backup service.

Revision Contents
Changeset List

PathSize
native/
cpp/
CommonCpp/
grpc/
protos/
14 lines
8 lines

Diff 13712

View Options

native/cpp/CommonCpp/grpc/protos/backup.proto

Loading...
View Options

native/cpp/CommonCpp/grpc/protos/blob.proto

Loading...