Page MenuHomePhabricator
Differential D5596

[services] Tunnelbroker - Removing the old API
ClosedPublic
Actions

Authored by • max on Nov 10 2022, 7:27 AM.
Tags
None
Referenced Files
F3509244: D5596.id18791.diff
Sat, Dec 21, 2:51 AM
F3509236: D5596.id18584.diff
Sat, Dec 21, 2:46 AM
F3509224: D5596.id18564.diff
Sat, Dec 21, 2:40 AM
F3509036: D5596.diff
Sat, Dec 21, 12:38 AM
Unknown Object (File)
Thu, Dec 19, 9:35 PM
Unknown Object (File)
Thu, Dec 19, 9:35 PM
Unknown Object (File)
Thu, Dec 19, 9:35 PM
Unknown Object (File)
Thu, Dec 19, 9:35 PM
View All Files
Subscribers
• abosh
ashoat
atul
tomek

Details

Reviewers
atul
tomek
• jon
Commits
rCOMMab3f2bce300c: [services] Tunnelbroker - Removing the old API
Summary

This diff removes the old legacy API methods from the Tunnelbroker API including the proto file and calls from the native codebase.
Old legacy API methods that should be removed:

  • CheckIfPrimaryDeviceOnline
  • BecomeNewPrimaryDevice
  • SendPong

These methods were added prior 0.1 and are not used anymore.

These diff changes take place in:

  • The protobuff file and its generated ones - by removing the API calls;
  • Native C++ network client-related codebase - by removing the API-related methods;
  • Tunnelbroker Rust gRPC server - by removing the empty handlers for the old API.

Linear task: ENG-1334

Test Plan

Services and native iOS and Android builds are successful.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• max created this revision.Nov 10 2022, 7:27 AM
• max held this revision as a draft.
Herald added a reviewer: • jon. · View Herald TranscriptNov 10 2022, 7:27 AM
Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript
Harbormaster failed remote builds in B13361: Diff 18318!Nov 10 2022, 7:31 AM
• max retitled this revision from [services] Tunnelbroker - Removing deprecated gRPC API to [services] Tunnelbroker - Removing the old API from the native client.Nov 18 2022, 4:14 AM
• max updated this revision to Diff 18563.Nov 18 2022, 4:16 AM

Removing the old API calls from the native client and network side.

Harbormaster failed remote builds in B13544: Diff 18563!Nov 18 2022, 4:17 AM
• max edited the summary of this revision. (Show Details)Nov 18 2022, 4:20 AM
• max edited the test plan for this revision. (Show Details)
• max edited reviewers, added: atul, tomek; removed: • jon.
Herald added a reviewer: • jon. · View Herald TranscriptNov 18 2022, 4:20 AM
• max edited the summary of this revision. (Show Details)Nov 18 2022, 4:23 AM
• max updated this revision to Diff 18564.Nov 18 2022, 4:25 AM

Removing the old API from the Tunnelbroker's gRPC Rust handlers.

• max retitled this revision from [services] Tunnelbroker - Removing the old API from the native client to [services] Tunnelbroker - Removing the old API.Nov 18 2022, 4:32 AM
• max edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B13545: Diff 18564.Nov 18 2022, 4:38 AM
• max published this revision for review.Nov 18 2022, 4:38 AM
• max updated this revision to Diff 18580.Nov 18 2022, 8:34 AM

Removing of the old structs from the protofile.

Harbormaster failed remote builds in B13561: Diff 18580!Nov 18 2022, 8:36 AM
• max updated this revision to Diff 18584.Nov 18 2022, 9:05 AM

Fixing building error.

Harbormaster failed remote builds in B13565: Diff 18584!Nov 18 2022, 9:11 AM
• max planned changes to this revision.Nov 18 2022, 10:06 AM
• max updated this revision to Diff 18593.Nov 18 2022, 10:42 AM

Fixing native builds.

Harbormaster completed remote builds in B13574: Diff 18593.Nov 18 2022, 10:55 AM
• max added a child revision: D5684: [services] Tunnelbroker - Removing of `get` and `send` methods from API and native codebase.Nov 18 2022, 11:36 AM
tomek accepted this revision.Nov 21 2022, 4:47 AM
tomek added inline comments.
native/android/app/src/cpp/GlobalNetworkSingletonJNIHelper.cpp
1–8 ↗(On Diff #18593)

I'm wondering if it makes sense to keep these empty files. On one hand we might need them again at some point, but reintroducing them shouldn't be expensive and maybe it is a better idea to delete them now.

services/tunnelbroker/src/server/mod.rs
46–47 ↗(On Diff #18593)

Is this comment still relevant?

atul accepted this revision.Nov 21 2022, 2:22 PM

Didn't do a super thorough review, but looks reasonable.

native/android/app/src/cpp/GlobalNetworkSingletonJNIHelper.cpp
1–8 ↗(On Diff #18593)

Agree that it'd be better to remove file altogether... it can always be found in the git history in the future if needed.

At a minimum I think we can remove GlobalNetworkSingleton.h since it's no longer being used?

This revision is now accepted and ready to land.Nov 21 2022, 2:22 PM
• max marked 3 inline comments as done.Nov 23 2022, 12:09 PM
• max added inline comments.
native/android/app/src/cpp/GlobalNetworkSingletonJNIHelper.cpp
1–8 ↗(On Diff #18593)

I'm wondering if it makes sense to keep these empty files.

it can always be found in the git history in the future if needed.

I'm thinking the same, we can always roll back in a git, so it's not a problem to remove it now.

At a minimum I think we can remove GlobalNetworkSingleton.h since it's no longer being used?

Yes, the GlobalNetworkSingleton.h and network-related C++ code is removed because it uses reactors and C++ gRPC which is removed later.

services/tunnelbroker/src/server/mod.rs
46–47 ↗(On Diff #18593)

Is this comment still relevant?

Yep, I'll remove it along with the send and get in a following D5684.

• max updated this revision to Diff 18791.Nov 23 2022, 12:20 PM
• max marked 2 inline comments as done.

Rebasing on master.

Harbormaster completed remote builds in B13711: Diff 18791.Nov 23 2022, 12:35 PM
• max updated this revision to Diff 18912.Nov 29 2022, 2:42 AM

Rebase and merge.

Harbormaster completed remote builds in B13819: Diff 18912.Nov 29 2022, 2:53 AM
Closed by commit rCOMMab3f2bce300c: [services] Tunnelbroker - Removing the old API (authored by • max). · Explain WhyNov 29 2022, 3:02 AM
This revision was automatically updated to reflect the committed changes.
• max added a commit: rCOMMab3f2bce300c: [services] Tunnelbroker - Removing the old API.

Revision Contents
Changeset List

PathSize
native/
android/
app/
src/
cpp/
9 lines
main/
java/
app/
comm/
android/
4 lines
fbjni/
4 lines
cpp/
CommonCpp/
NativeModules/
InternalModules/
3 lines
1 line
4 lines
grpc/
5 lines
49 lines
ios/
Comm/
12 lines
services/
tunnelbroker/
src/
server/
21 lines
shared/
protos/
_generated/
43 lines

Diff 18918

View Options

native/android/app/src/cpp/GlobalNetworkSingletonJNIHelper.cpp

Loading...
View Options

native/android/app/src/main/java/app/comm/android/CommNotificationsHandler.java

Loading...
View Options

native/android/app/src/main/java/app/comm/android/fbjni/NetworkModule.java

Loading...
View Options

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalNetworkSingletonJNIHelper.h

Loading...
View Options

native/cpp/CommonCpp/NativeModules/InternalModules/NetworkModule.h

Loading...
View Options

native/cpp/CommonCpp/NativeModules/InternalModules/NetworkModule.cpp

Loading...
View Options

native/cpp/CommonCpp/grpc/Client.h

Loading...
View Options

native/cpp/CommonCpp/grpc/Client.cpp

Loading...
View Options

native/ios/Comm/AppDelegate.mm

Loading...
View Options

services/tunnelbroker/src/server/mod.rs

Loading...
View Options

shared/protos/_generated/tunnelbroker.grpc.pb.h

Loading...
View Options

shared/protos/_generated/tunnelbroker.grpc.pb.cc

Loading...
View Options

shared/protos/_generated/tunnelbroker.pb.h

Loading...
View Options

shared/protos/_generated/tunnelbroker.pb.cc

Loading...
View Options

shared/protos/tunnelbroker.proto

Loading...