[services] Lib - Tunnelbroker - Use database manager
ClosedPublic
Actions

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

Details

Reviewers

tomek
• max

Commits

rCOMMabf5c5701400: [services] Lib - Tunnelbroker - Use database manager

Summary

Depends on D3989

Inheriting from DatabaseManagerBase in the tunnelbroker to avoid the boilerplate and reuse common methods.

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, 11:27 AM

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

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

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

• karol added reviewers: tomek, • max.

• karol added a parent revision: D3989: [services] Lib - Tunnelbroker - Use sources.

Harbormaster completed remote builds in B8982: Diff 12492.May 10 2022, 11:45 AM

• karol requested review of this revision.May 10 2022, 11:45 AM

• karol added 1 blocking reviewer(s): • max.May 10 2022, 1:50 PM

• max requested changes to this revision.May 11 2022, 6:03 AM

• max added inline comments.

services/tunnelbroker/src/Database/DatabaseManager.cpp
69–72 ↗	(On Diff #12492)	We should not return silently without any errors here in case of the removing item is not found. We are catching the throw errors at the upper level (grpc handler) to make the error messages descriptive and handle some occurrences where we need to stop operations in case the item was removed before. In case we use this return we must use some debug information that the removing item was not found. But I think it's better not to use it here.
102–105 ↗	(On Diff #12492)	Same as above.
131–134 ↗	(On Diff #12492)	Same as above.
205–208 ↗	(On Diff #12492)	Same as above.

This revision now requires changes to proceed.May 11 2022, 6:03 AM

• karol added inline comments.May 11 2022, 9:06 AM

services/tunnelbroker/src/Database/DatabaseManager.cpp
69–72 ↗	(On Diff #12492)	I think you're right and we should attach information on whether the item's been removed or not. I think we can throw.

D3999

ashoat mentioned this in D3999: [services] consistently return when trying to remove non-existent item.May 12 2022, 1:02 AM

• karol planned changes to this revision.May 12 2022, 9:08 AM

address feedback

Harbormaster completed remote builds in B9031: Diff 12572.May 12 2022, 9:14 AM

tomek requested changes to this revision.May 12 2022, 9:38 AM

tomek added inline comments.

services/tunnelbroker/src/Database/DatabaseManager.cpp
69–72 ↗	(On Diff #12492)	I don't agree. If we wanted to remove an item and it can't be found, it is a success: the item is not there. We can include some info for a client with an explanation, but what a client could do with that? The goal was for an item to not exist... and this is what happened. @geekbrother what do you think a client can do about this error?

This revision now requires changes to proceed.May 12 2022, 9:38 AM

I'll request review to make sure this is in the @geekbrother's queue.

services/tunnelbroker/src/Database/DatabaseManager.cpp
69–72 ↗	(On Diff #12492)	I'm not sure if grpc handles something like this. I think there's just `error_message` and it may not be possible to return a success + message. I honestly do not mind which path we take. We can get back to just `return`ing. The goal is to land this and proceed. @geekbrother can you answer to this?

I have very little context on what exactly this API is deleting, but I generally agree with @palys-swm's perspective – making operations "idempotent" (same result if you run it twice in a row) is generally a good idea

I also don't think it's that important to include a message. I think you can just return success

Ok, so I don't mind, Tomek and Ashoat vote for return, so basically whichever path @geekbrother takes, we'll proceed with return anyways. I'm going to implement this. Thx.

return instead of throwing

Harbormaster failed remote builds in B9074: Diff 12620!May 13 2022, 9:24 AM

tomek accepted this revision.May 13 2022, 9:25 AM

• max accepted this revision.May 13 2022, 11:18 AM

• max added inline comments.

services/tunnelbroker/src/Database/DatabaseManager.cpp
69–72 ↗	(On Diff #12492)	Maybe it was an overengineering here. My thoughts were about if the connection to AWS disappear before this call, didn't we just return success on it. But we are also throwing an error inside the `DatabaseManager::innerFindItem(...)` and there is no big chance for this situation.