Page MenuHomePhabricator
Differential D5206

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

Authored by • max on Sep 21 2022, 3:37 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, May 2, 3:34 PM
Unknown Object (File)
Thu, May 2, 3:34 PM
Unknown Object (File)
Thu, May 2, 3:33 PM
Unknown Object (File)
Thu, May 2, 3:13 PM
Unknown Object (File)
Tue, Apr 30, 5:31 PM
Unknown Object (File)
Tue, Apr 30, 5:31 PM
Unknown Object (File)
Tue, Apr 30, 5:31 PM
Unknown Object (File)
Tue, Apr 30, 5:31 PM
View All 64 Files
Subscribers
• abosh
ashoat
atul
tomek

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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
• jon accepted this revision.Sep 22 2022, 12:14 PM

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 ↗(On Diff #16968)

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 ↗(On Diff #16968)

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 ↗(On Diff #16968)

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 ↗(On Diff #16968)

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 ↗(On Diff #16968)

I agree, this would be easier to review as one diff imo

Ok, I'll take it into account!

185–192 ↗(On Diff #16968)

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

PathSize
services/
tunnelbroker/
src/
Service/
10 lines

Diff 17062

View Options

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp

Loading...