[services] Lib - Tunnelbroker - Use sources
ClosedPublic
Actions

Authored by • karol on May 10 2022, 4:27 AM.

Details

Reviewers

tomek
• max

Commits

rCOMM9640b4d4899a: [services] Lib - Tunnelbroker - Use sources

Summary

Depends on D3988

Linking header/source files from the "lib" directory to the tunnelbroker. I had to perform some operations in order to avoid having compile errors. Maybe I missed something but I don't think it's possible to have a smaller changeset after attaching the sources from "lib".

Test Plan

You should be able to build tunnelbroker properly

cd services
yarn run-tunnelbroker-service

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• karol created this revision.May 10 2022, 4:27 AM

Herald added subscribers: • abosh, atul, • adrian and 2 others. · View Herald TranscriptMay 10 2022, 4:27 AM

Harbormaster completed remote builds in B8981: Diff 12491.May 10 2022, 4:40 AM

• karol requested review of this revision.May 10 2022, 4:40 AM

• karol edited the summary of this revision. (Show Details)May 10 2022, 4:42 AM

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

• karol added reviewers: tomek, • max.

• karol added a parent revision: D3988: [services] Lib - Tunnelbroker - Use scripts.

• karol added a child revision: D3990: [services] Lib - Tunnelbroker - Use database manager.

• karol added 1 blocking reviewer(s): • max.May 10 2022, 6:49 AM

• max requested changes to this revision.May 10 2022, 11:21 PM

• max added inline comments.

services/tunnelbroker/CMakeLists.txt
60 ↗	(On Diff #12491)	Nice catch!
services/tunnelbroker/src/Database/DeviceSessionItem.cpp
86 ↗	(On Diff #12491)	Why we are not using `std::sring` here? Seems we are return a simple string.
services/tunnelbroker/src/Database/DeviceSessionItem.h
34–35 ↗	(On Diff #12491)	Maybe we should change this to use `Name` and `Value` suffix. It's not obvious now what it returns.
services/tunnelbroker/src/Database/MessageItem.h
32 ↗	(On Diff #12491)	Same question here: Does it seem like `PrimaryKey` can have only a string, why don't you use it? Despite `PrimaryKeyValue` is not. If the `PrimaryKeyValue` is a complex structure we should name it Values or Struct. It seems the naming needs to be changed, it's kind of misleading here at my look.

This revision now requires changes to proceed.May 10 2022, 11:21 PM

• karol requested review of this revision.May 11 2022, 1:55 AM

• karol added inline comments.

services/tunnelbroker/src/Database/DeviceSessionItem.cpp
86 ↗	(On Diff #12491)	Maybe in this case we do return a simple string but this is only because you don't use sort keys. If we ever want to use them, this will come beneficial to us. It's better to use methods from `Item.h`.
services/tunnelbroker/src/Database/DeviceSessionItem.h
34–35 ↗	(On Diff #12491)	We can do this, but I think it would be better to do this in a follow-up as it appears in blob, backup, tunnelbroker, and lib, so it's a bit beyond this revision.
services/tunnelbroker/src/Database/MessageItem.h
32 ↗	(On Diff #12491)	Same question here: Does it seem like `PrimaryKey` can have only a string, why don't you use it? Despite `PrimaryKeyValue` is not. Same answer as above. Please, note how the inheritance of `Item.h` works across services. If the `PrimaryKeyValue` is a complex structure we should name it Values or Struct. It seems the naming needs to be changed, it's kind of misleading here at my look. Where does this come from? Sorry, but this sounds weird. `PrimaryKeyValue` is one of the simples structs possible: 2 fields, 2 constructors. Most of the classes in our codebase are more complex than that. Similarly, you could say we should rename all classes like this, so instead `DatabaseManager` we should do `DatabaseManagerClass` (inb4: class is almost the same thing as struct). I think naming's fine.

ashoat added inline comments.May 11 2022, 5:58 PM

services/tunnelbroker/src/Database/DeviceSessionItem.h
34–35 ↗	(On Diff #12491)	When a diff reviewer gives you a comment that applies to other places as well, I think you can reduce the amount of work for the reviewer (who has to track your work across diffs) by doing one of two things: (Preferred) respond to the request immediately by making the change Then either submit a diff with the changes to other files, and then link it from the original diff Or create a Linear task for the changes to other files, and link it from the original diff Either way you should be linking something, and either way you should write down the list of files somewhere (Alternative if making the change in only one place breaks the build / causes a crash / etc.) Don't update the current diff but do the other thing (either link the Linear task or the follow-up diff) Some high-level rules: If you agree with the request, you should do whatever possible to respond to the request in the current diff You should never defer work without linking either a follow-up diff or a precise Linear task that directly talks about the thing being requested

• karol planned changes to this revision.May 12 2022, 1:30 AM

• karol added inline comments.May 12 2022, 1:38 AM

services/tunnelbroker/src/Database/DeviceSessionItem.h
34–35 ↗	(On Diff #12491)	ok, sorry you're right

D4013

tomek accepted this revision.May 12 2022, 2:24 AM

tomek added inline comments.

services/tunnelbroker/src/Database/DeviceSessionItem.cpp
86 ↗	(On Diff #12491)	Another benefit is that we can easily distinguish between `PrimaryKey` and `PrimaryKeyValue`. If we used just strings, it would be easier to make a mistake.
services/tunnelbroker/src/Tools/AwsTools.cpp
19–28 ↗	(On Diff #12491)	Was this function unused, or this diff changed something which allows us to remove it?