Paths

Table of Contentst

[services] Tunnelbroker - Update handling of the `newNotifyToken` in `Get` stream request
ClosedPublic
Actions

Authored by • max on Sep 21 2022, 3:37 PM.

Details

Reviewers

• jon
tomek

Commits

rCOMM8f4cc18c8948: [services] Tunnelbroker - Update handling of the `newNotifyToken` in `Get`…

Summary

This diff introduces handling of the newNotifyToken in Get stream request.
When the device has an invalid or expired notification token we are requesting a new token from the client (ref to D5207).
The client gets back with the new token in Get stream request when starts listening for new messages (getting messages) from the Tunnelbroker.

This is a part of the stack.
This approach was used keeping in mind the transition to the bidirectional stream soon.

Related Linear task: ENG-1782 and ENG-1816

Test Plan

Successfully built the service and passed unit and integration tests.

Diff Detail

Repository

rCOMM Comm

Branch

handler-update-device-token

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Sep 21 2022, 3:37 PM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptSep 21 2022, 3:37 PM

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

• max added parent revisions: D5180: [services] Tunnelbroker - Updating Protobuf with new token request and token update, D5179: [services] Tunnelbroker - Add `updateSessionItemDeviceToken` database method.Sep 21 2022, 3:38 PM

Harbormaster completed remote builds in B12359: Diff 16968.Sep 21 2022, 3:47 PM

• max edited the summary of this revision. (Show Details)Sep 21 2022, 4:08 PM

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

• max added a reviewer: tomek.

• max published this revision for review.Sep 21 2022, 4:11 PM

• max mentioned this in D5207: [services] Tunnelbroker - Update sending of the `newNotifyTokenRequired` in `Get` stream request.

• max added a child revision: D5207: [services] Tunnelbroker - Update sending of the `newNotifyTokenRequired` in `Get` stream request.

• max mentioned this in D5210: [services] Tunnelbroker - Updating `Send` handler with using of the Rust notification library.Sep 22 2022, 5:12 AM

LGTM, as long as https://phab.comm.dev/D5207 lands at the same time.

For dealing with more cases in the future, I think we should eventually do something like:

if (request->has_newnotifytoken()) {
  handle_new_notify_token(request);
} (request->has_ping()) {
  handle_ping(request);
}

Where we deconstruct some of the information, and able to write functions which deal with more specific scenarios. May also allow for some unittests, if it's just "data in, data out".

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
185–192	concerned about this being partial (only handles , but looks like https://phab.comm.dev/D5207 handles the new token logic, not sure if in the future, we should just combine these types of diffs. Although your summary mentions D5207, I would expect for a commit to handle more of the vertical slice. Maybe it's just me, but I'm not a big fan of "intermediate diffs". Not sure how @ashoat feels.

ashoat added inline comments.Sep 22 2022, 12:25 PM

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
185–192	I agree, this would be easier to review as one diff imo

• jon added inline comments.Sep 22 2022, 1:03 PM

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
185–192	Meant to say "only handles the database write failure"

tomek accepted this revision.Sep 23 2022, 5:02 AM

This revision is now accepted and ready to land.Sep 23 2022, 5:02 AM

• max mentioned this in rCOMMa206a33acdc2: [services] Tunnelbroker - Updating `Send` handler with using of the Rust….Sep 23 2022, 12:46 PM

Closed by commit rCOMM8f4cc18c8948: [services] Tunnelbroker - Update handling of the `newNotifyToken` in `Get`… (authored by • max). · Explain WhySep 23 2022, 12:49 PM

This revision was automatically updated to reflect the committed changes.

• max marked an inline comment as done.

• max added a commit: rCOMM8f4cc18c8948: [services] Tunnelbroker - Update handling of the `newNotifyToken` in `Get`….

• max marked 2 inline comments as done.Sep 23 2022, 12:51 PM

• max added inline comments.

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
185–192	concerned about this being partial (only handles , but looks like https://phab.comm.dev/D5207 handles the new token logic, not sure if in the future, we should just combine these types of diffs. Although your summary mentions D5207, I would expect for a commit to handle more of the vertical slice. It's split by the functionality actions: we are updating, we are requesting. This follows the phabricator convention to make smaller diffs but each diff should not break the building of the app. Maybe it's just me, but I'm not a big fan of "intermediate diffs". Sometimes it's necessary. I think the main decision when splitting into smaller chunks/diffs is to think as a reviewer and how would be better to review it. Maybe sometimes is too much splitting. Thanks for your thoughts @jon !
185–192	I agree, this would be easier to review as one diff imo Ok, I'll take it into account!
185–192	Meant to say "only handles the database write failure" We are updating the new token in the database here and handling the possible error on this operation.

• max mentioned this in rCOMM3e557c0336aa: [services] Tunnelbroker - Update sending of the `newNotifyTokenRequired` in….Sep 23 2022, 1:13 PM

Revision Contents
Changeset List

Path

Size

services/

tunnelbroker/

src/

Service/

TunnelbrokerServiceImpl.cpp

10 lines

Diff 16968

View Options

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp

[services] Tunnelbroker - Update handling of the `newNotifyToken` in `Get` stream requestClosedPublicActions