[services] Tunnelbroker - Wrap gRPC Get message response into a function in Tunnelbroker
ClosedPublic
Actions

Authored by • max on May 4 2022, 8:08 AM.

Details

Reviewers

• karol
varun
ashoat

Commits

rCOMMe9473c9faa8f: [services] Tunnelbroker - Wrap gRPC Get message response into a function in…

Summary

Wrap gRPC Get handler message response into a function because the same logic exists for sending a response on a new message from AMQP and database.
We don't need to repeat this part of the code and must wrap it into a reusable function.
As this part is for Get handler specific, there is no reason to make this a separate class private function. We can move it to internal lambda.

Linear task: ENG-1043

Test Plan

Run yarn run-tunnelbroker-service and successfully built the service.

Diff Detail

Repository

rCOMM Comm

Branch

wrap-grpc-get-response

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.May 4 2022, 8:08 AM

Herald added subscribers: • abosh, • karol, atul and 3 others. · View Herald TranscriptMay 4 2022, 8:08 AM

• max edited the summary of this revision. (Show Details)May 4 2022, 8:08 AM

• max edited the test plan for this revision. (Show Details)May 4 2022, 8:11 AM

• max added reviewers: • karol, varun, ashoat.

• max added parent revisions: D3808: [services] Tunnelbroker - Message expiration fulfill moved to database manager's insert, D3833: [services] Tunnelbroker - Put and get messages from database in grpc handler.

Harbormaster returned this revision to the author for changes because remote builds failed.May 4 2022, 8:12 AM

Harbormaster failed remote builds in B8776: Diff 12223!

• max requested review of this revision.May 4 2022, 11:01 AM

Assuming the android issues are unrelated. @geekbrother, in the future it's better to restart the CI build instead of ignoring it and requesting review anyways. That said, sometimes the build keeps failing for the same reason... in that case, requesting review anyways is the best option

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
217	Let's use the verb `respond` instead of the noun `response`

This revision is now accepted and ready to land.May 4 2022, 11:18 AM

Update the lambda name to respondToWriter.

Harbormaster completed remote builds in B8792: Diff 12241.May 4 2022, 12:03 PM

In D3914#109574, @ashoat wrote:

Assuming the android issues are unrelated. @geekbrother, in the future it's better to restart the CI build instead of ignoring it and requesting review anyways. That said, sometimes the build keeps failing for the same reason... in that case, requesting review anyways is the best option

I've checked the Buildkite log before re-requesting the review. The error was unrelated to the services and seems it was fixed after. That's why didn't trigger it one more time.

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
217	Let's use the verb `respond` instead of the noun `response` Fixed. The name was changed.

• karol accepted this revision.May 5 2022, 6:24 AM

I've checked the Buildkite log before re-requesting the review. The error was unrelated to the services and seems it was fixed after. That's why didn't trigger it one more time.

In that case it can be good to clarify that on the diff. But again, it's always preferable to trigger it again. Only if it repeatedly fails for an unrelated reason should you "Request Review", and in that scenario you should ping @atul to make sure he knows something is wrong with the CI.