Details

Reviewers

• jon
marcin
tomek

Commits

rCOMM32d6628a1db5: [services] Tunnelbroker - Changes to store the `deviceType` as an int instead…

Summary

This diff introduces changes to the type for deviceType field from a string to an integer.
The deviceType is an enum type in our proto file. The values come from the gRPC requests as an integer enum representation, Rust Tonic proto file compiler also creates the enum representation as an integer.

Before switching to Rust we are using the string as a storing type of the enum value in the database because of the enum matching was done using strings on the C++ side.
Now it doesn't make sense to store the enum value as a string in a database, because the matching of the enum in Rust uses integer representation as well as in gRPC implementation itself.

Linear task: ENG-2059

Test Plan

The legacy C++ codebase is successfully built.
The usage of the functions is in D5490 and it successfully built

Diff Detail

Repository

rCOMM Comm

Branch

change-deviceid-as-int

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Nov 8 2022, 10:00 AM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptNov 8 2022, 10:00 AM

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

Harbormaster failed remote builds in B13276: Diff 18218!Nov 8 2022, 10:02 AM

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

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

• max added a reviewer: marcin.

Adding changes in gtests.

• max published this revision for review.Nov 8 2022, 10:22 AM

• max added a child revision: D5490: [services] Tunnelbroker - Adding of gRPC `NewSession` wrapper implementation.

• max added a reviewer: tomek.Nov 8 2022, 10:31 AM

Harbormaster failed remote builds in B13280: Diff 18222!Nov 8 2022, 10:32 AM

• max mentioned this in D5528: [services] Tunnelbroker - Adding of the client authentication by the `sessionID` from metadata.Nov 8 2022, 10:58 AM

• max mentioned this in D5536: [services] Tunnelbroker - Adding of requesting a new notification token if empty.Nov 8 2022, 3:56 PM

Harbormaster completed remote builds in B13280: Diff 18222.Nov 8 2022, 4:43 PM

tomek requested changes to this revision.Nov 9 2022, 8:22 AM

tomek added inline comments.

services/tunnelbroker/src/libcpp/src/Database/DatabaseManager.cpp
43–44 ↗	(On Diff #18222)	This confuses me a bit. Why do we need to transform it to string?
services/tunnelbroker/src/libcpp/src/Database/DeviceSessionItem.cpp
64–66 ↗	(On Diff #18222)	Why this is different then reading `checkpointTime`?
services/tunnelbroker/src/libcpp/src/Database/DeviceSessionItem.h
16 ↗	(On Diff #18222)	Is `size_t` the right type? How many bytes does this value occupy in the db?
services/tunnelbroker/src/libcpp/test/DatabaseManagerTest.cpp
144 ↗	(On Diff #18222)	Is there a way to use an enum instead of magic numbers?

This revision now requires changes to proceed.Nov 9 2022, 8:22 AM

• max requested review of this revision.Nov 16 2022, 4:24 AM

• max marked 4 inline comments as done.

• max added inline comments.

services/tunnelbroker/src/libcpp/src/Database/DatabaseManager.cpp
43–44 ↗	(On Diff #18222)	This confuses me a bit. Why do we need to transform it to string? This a long story, but the AWS SDK storing numbers (and returning numbers as strings) even when we are using GetN (get number). That's confusing and we have discussions with Karol and you last year I think, about how to solve this, but that's how AWS-CPP SDK works. Numbers as string 🤷‍♂️
services/tunnelbroker/src/libcpp/src/Database/DeviceSessionItem.cpp
64–66 ↗	(On Diff #18222)	Why this is different then reading `checkpointTime`? GetS and GetN both returns string despite the fact that GetN - Get Number. I just followed the convention that despite we got the string we expecting this to be converted into the number. But I can use `GetS` and there are no differences.
services/tunnelbroker/src/libcpp/src/Database/DeviceSessionItem.h
16 ↗	(On Diff #18222)	Is `size_t` the right type? How many bytes does this value occupy in the db? We are storing a small number here but I've used `size_t` for compatibility with the Rust CXX types. We can use here some smaller int but later we still should convert it into compatible `size_t` bigger type and static casting is costly (I think more costly than just storing in a bigger int).
services/tunnelbroker/src/libcpp/test/DatabaseManagerTest.cpp
144 ↗	(On Diff #18222)	Is there a way to use an enum instead of magic numbers? This is a gRPC enum from the proto file. As we've changed the gRPC server to Rust we've removed the proto file compilation into C++ headers, the only way to use the enum here is to redefine it manually. I don't think this is a good idea, because of the switching to Rust and we should maintain this manually added enum according to the proto file in this legacy C++ code. I think the better way here is to add a comment describing what the number is and where it came from.

Adding a comment to the DeviceTypes enum mubers.

Harbormaster completed remote builds in B13464: Diff 18463.Nov 16 2022, 4:42 AM

tomek requested changes to this revision.Nov 17 2022, 2:33 AM

tomek added inline comments.

services/tunnelbroker/src/libcpp/src/Database/DeviceSessionItem.cpp
64–66 ↗	(On Diff #18222)	We declared `deviceType` as `size_t` but convert to it using `stoll` which returns `long long`. Can we make this more consistent?
services/tunnelbroker/src/libcpp/test/DatabaseManagerTest.cpp
144 ↗	(On Diff #18222)	I don't think this is a good idea, because of the switching to Rust and we should maintain this manually added enum according to the proto file in this legacy C++ code. I think the better way here is to add a comment describing what the number is and where it came from. I strongly disagree. So now, if the gRPC enum changes, he have to search the codebase for all its usages and update it. If we had a const / enum defined, we could update only it. There's no reason to encode something as a comment when we can have a named constant with it.

This revision now requires changes to proceed.Nov 17 2022, 2:33 AM

• max planned changes to this revision.Nov 17 2022, 5:18 AM

Adding of the DeviceTypes enum. Change to use stoul to convert from the database's integer to device type.

• max marked 2 inline comments as done.Nov 17 2022, 6:19 AM

• max added inline comments.

services/tunnelbroker/src/libcpp/src/Database/DeviceSessionItem.cpp
64–66 ↗	(On Diff #18222)	We declared `deviceType` as `size_t` but convert to it using `stoll` which returns `long long`. Can we make this more consistent? Good catch, thanks @tomek! I think more closest to the `size_t` here is to use `stoul` instead. I've made this change. We can't use a static cast here like `static_cast<size_t>` or any other castings other than `st` functions. Other methods require streams and additional variables which is overkill here. We can possibly use a `lexical_cast` from boost, but we don't want to add an additional dependency for this as we already have a `sto` functions in the standard library.
services/tunnelbroker/src/libcpp/test/DatabaseManagerTest.cpp
144 ↗	(On Diff #18222)	I don't think this is a good idea, because of the switching to Rust and we should maintain this manually added enum according to the proto file in this legacy C++ code. I think the better way here is to add a comment describing what the number is and where it came from. I strongly disagree. So now, if the gRPC enum changes, he have to search the codebase for all its usages and update it. If we had a const / enum defined, we could update only it. There's no reason to encode something as a comment when we can have a named constant with it. Ok, that makes sense. I've added an `enum DeviceTypes` with the devices types, now we are using an enum without numbers.

Harbormaster completed remote builds in B13516: Diff 18525.Nov 17 2022, 6:26 AM

tomek accepted this revision.Nov 18 2022, 3:04 AM

tomek added inline comments.

services/tunnelbroker/src/libcpp/src/Database/DeviceSessionItem.h
36 ↗	(On Diff #18525)	Do we really skip `2`?

This revision is now accepted and ready to land.Nov 18 2022, 3:04 AM

Fixing enum discriminant number for the KEYSERVER.

• max marked an inline comment as done.Nov 19 2022, 12:18 PM

• max added inline comments.

services/tunnelbroker/src/libcpp/src/Database/DeviceSessionItem.h
36 ↗	(On Diff #18525)	Do we really skip `2`? No, that's a bug! Thanks, @tomek for catching this!

Harbormaster completed remote builds in B13592: Diff 18619.Nov 19 2022, 12:30 PM

Closed by commit rCOMM32d6628a1db5: [services] Tunnelbroker - Changes to store the `deviceType` as an int instead… (authored by • max). · Explain WhyNov 19 2022, 12:41 PM

This revision was automatically updated to reflect the committed changes.

• max marked an inline comment as done.

• max added a commit: rCOMM32d6628a1db5: [services] Tunnelbroker - Changes to store the `deviceType` as an int instead….

[services] Tunnelbroker - Changes to store the `deviceType` as an int instead of string
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 18619

services/tunnelbroker/src/libcpp/src/Database/DatabaseManager.cpp

services/tunnelbroker/src/libcpp/src/Database/DeviceSessionItem.h

services/tunnelbroker/src/libcpp/src/Database/DeviceSessionItem.cpp

services/tunnelbroker/src/libcpp/test/DatabaseManagerTest.cpp

[services] Tunnelbroker - Changes to store the `deviceType` as an int instead of stringClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 18619

services/tunnelbroker/src/libcpp/src/Database/DatabaseManager.cpp

services/tunnelbroker/src/libcpp/src/Database/DeviceSessionItem.h

services/tunnelbroker/src/libcpp/src/Database/DeviceSessionItem.cpp

services/tunnelbroker/src/libcpp/test/DatabaseManagerTest.cpp

[services] Tunnelbroker - Changes to store the `deviceType` as an int instead of string
ClosedPublic
Actions

Revision Contents
Changeset List