[services] Tunnelbroker - Update Tunnelbroker protobuf file with ping and new device token in bidirectional stream
ClosedPublic
Actions

Authored by • max on Oct 13 2022, 8:07 AM.

Details

Reviewers

• jon
tomek
varun
ashoat

Commits

rCOMM20586527efc1: [services] Tunnelbroker - Update Tunnelbroker protobuf file with ping and new…

Summary

This diff introduces an update of Tunnelbroker protobuf bidirectional stream MessagesStream with the ping and device token fields.

In ENG-1766 we created pinging messages to make sure the client is online. Currently, we are using it in a unidirectional Get, but we should add it to the bidirectional MessagesStream as it will replace the Send and Get methods.

In ENG-1782 we've added the mechanism of requesting new device notification tokens. Currently, we are requesting it in the Get message and expecting it in Send from the client. These two fields must be merged into the bidirectional MessagesStream because Get and Send will be deprecated in a favor of one bidirectional MessagesStream.

Related Linear task: ENG-2043

Test Plan

Protobuf file compilation is successful.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• max created this revision.Oct 13 2022, 8:07 AM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptOct 13 2022, 8:07 AM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript

• max edited the summary of this revision. (Show Details)Oct 13 2022, 8:09 AM

• max edited the test plan for this revision. (Show Details)

• max added reviewers: tomek, varun.

• max edited the summary of this revision. (Show Details)

• max published this revision for review.Oct 13 2022, 8:15 AM

Harbormaster completed remote builds in B12774: Diff 17544.Oct 13 2022, 8:17 AM

This is probably what I would do, but @tomek should still review

This revision now requires changes to proceed.Oct 13 2022, 1:09 PM

meant to accept, misclick

This revision now requires review to proceed.Oct 13 2022, 1:09 PM

Adding @ashoat because this diff changes proto files

shared/protos/tunnelbroker.proto
83–84 ↗	(On Diff #17544)	Are we planning to remove these later?

Question inline

shared/protos/tunnelbroker.proto
134 ↗	(On Diff #17544)	Don't we also need a `pong` back from the client?

This revision now requires changes to proceed.Oct 14 2022, 6:27 AM

• max requested review of this revision.Oct 14 2022, 11:20 AM

• max marked 2 inline comments as done.

• max added inline comments.

shared/protos/tunnelbroker.proto
83–84 ↗	(On Diff #17544)	Are we planning to remove these later? Yes, we have a task to remove the old API.
134 ↗	(On Diff #17544)	Don't we also need a `pong` back from the client? We don't need a `pong` from the client to make sure that the client is listening to the channel. The discussion was in ENG-1766 and D5174. In a short: we are pinging to make sure the channel is still open because the gRPC doesn't check for keep-alive by default. In case the client is not available he can't receive the ping and the `write()` to the channel will fail, in this case, we are marking the device as offline. For the messages processing confirmation, we already have processedMessages. But it doesn't makes sense for pinging. Also, there is a possibility that we will use the gRPC internal keep-alive mechanism and this `ping` can be removed after. The task for tracking this is ENG-1842.

Okay, makes sense

This revision is now accepted and ready to land.Oct 14 2022, 12:00 PM

tomek added inline comments.Oct 17 2022, 1:15 AM

shared/protos/tunnelbroker.proto
83–84 ↗	(On Diff #17544)	That task was created to handle `CheckIfPrimaryDeviceOnline`, `BecomeNewPrimaryDevice` and `SendPong`. This field was introduced less than a month ago so the original task definitely wasn't about removing it. If we want to handle removing these fields there, please update the task or its parent so that it's clear that the scope has changed.

• max marked 3 inline comments as done.Oct 17 2022, 3:43 AM

• max added inline comments.

shared/protos/tunnelbroker.proto
83–84 ↗	(On Diff #17544)	That task was created to handle `CheckIfPrimaryDeviceOnline`, `BecomeNewPrimaryDevice` and `SendPong`. This field was introduced less than a month ago so the original task definitely wasn't about removing it. If we want to handle removing these fields there, please update the task or its parent so that it's clear that the scope has changed. That's fair! I've updated the parent task for the old API cleanup and updating the native client with the recent changes. Thanks, @tomek!