Details

Reviewers

tomek
• max
ashoat
• jim
varun

Commits

rCOMM0c0a34fab719: [services] Move tools to their namespace

Summary

Depends on D4032

Move all tools to the ::tools namespace

Test Plan

Services still build

Diff Detail

Repository

rCOMM Comm

Branch

attachments

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• karol created this revision.May 13 2022, 2:28 AM

Herald added subscribers: • abosh, atul, • adrian and 2 others. · View Herald TranscriptMay 13 2022, 2:28 AM

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

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

• karol added a reviewer: tomek.

• karol added a parent revision: D4032: [services] Backup - use generateHolder for new log.

Harbormaster returned this revision to the author for changes because remote builds failed.May 13 2022, 2:51 AM

Harbormaster failed remote builds in B9084: Diff 12630!

fix buld

Harbormaster completed remote builds in B9087: Diff 12633.May 13 2022, 3:20 AM

• karol requested review of this revision.May 13 2022, 3:20 AM

Adding @geekbrother since he seems interested in this work, based on discussion in ENG-976

I think we need to distinguish service-specific namespace for tools and GlobalTools.
We already have tools:: namespace in the tunnelbroker specific tools at services/tunnelbroker/src/Tools/Tools[.cpp,.h].
The good idea is to use something like globalTools:: for the services/lib/src/GlobalTools[.cpp,.h]. Or we can change the namespace for the tunnelbroker specific tools at services/tunnelbroker/src/Tools/Tools[.cpp,.h] to tunnelbrokerTools:: and same for blob and backup services tools.

This revision now requires changes to proceed.May 14 2022, 12:38 PM

I think it's better to have the same namespace. Maybe someone else can take a vote here? Adding some people to the review

How about tools:: (or comm::tools:: or whatever) for the global tools, and tunnelbroker::tools:: (or comm::tunnelbroker::tools:: or whatever) for Tunnelbroker-specific tools?

Looks like we have comm::network::tools here, does that really make the most sense for something like services/lib/src/GlobalTools.h? Not sure what it has to do with "network"... passing back to @karol-bisztyga with this question

This revision now requires changes to proceed.May 16 2022, 1:09 PM

AFAIR network was introduced when we started working on services. I think we agreed that comm::network would be a good namespace for them. Tomek already pointed out that we may want to refactor this. This is the task for that: https://linear.app/comm/issue/ENG-1162/think-of-the-best-namespace-name.

As for the tools, looks like I'm outvoted. So let's go with tools for global tools and tunnelbroker::tools for tunnelbroker.

• karol added a child revision: D4074: [services] Backup - return log id of the newly created log to the client.May 19 2022, 3:21 AM

Okay fair point, this is best covered as a separate task/diff since it impacts other diffs / other files. Accepting to unblock, and thanks for creating a task!

This revision now requires review to proceed.May 19 2022, 12:34 PM

Thanks, Karol! Following the ENG-1162.

This revision is now accepted and ready to land.May 19 2022, 12:47 PM

rebase

Harbormaster failed remote builds in B9377: Diff 12994!May 23 2022, 2:55 AM

fix build

Harbormaster failed remote builds in B9379: Diff 12996!May 23 2022, 3:01 AM

fix build

Harbormaster completed remote builds in B9380: Diff 12997.May 23 2022, 3:27 AM

• max requested changes to this revision.May 23 2022, 4:34 AM

• max added inline comments.

services/tunnelbroker/test/DatabaseManagerTest.cpp
137 ↗	(On Diff #12997)	I think we need to use `tools::generateUUID()` here.

This revision now requires changes to proceed.May 23 2022, 4:34 AM

update

Harbormaster completed remote builds in B9422: Diff 13044.May 24 2022, 2:25 AM

• max accepted this revision.May 25 2022, 3:23 AM

This revision is now accepted and ready to land.May 25 2022, 3:23 AM

Closed by commit rCOMM0c0a34fab719: [services] Move tools to their namespace (authored by • karol). · Explain WhyMay 25 2022, 3:25 AM

This revision was automatically updated to reflect the committed changes.

• karol added a commit: rCOMM0c0a34fab719: [services] Move tools to their namespace.

[services] Move tools to their namespace
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 12633

services/backup/src/Constants.h

services/backup/src/DatabaseManager.cpp

services/backup/src/Reactors/server/CreateNewBackupReactor.cpp

services/backup/src/Reactors/server/SendLogReactor.cpp

services/backup/src/Tools.h

services/backup/src/Tools.cpp

services/backup/test/DatabaseManagerTest.cpp

services/blob/src/AwsS3Bucket.cpp

services/blob/src/BlobServiceImpl.cpp

services/blob/src/Constants.h

services/blob/src/DatabaseManager.cpp

services/blob/src/Reactors/server/GetReactor.h

services/blob/src/Reactors/server/PutReactor.h

services/blob/src/S3Tools.cpp

services/blob/src/Tools.h

services/blob/src/Tools.cpp

services/blob/test/MultiPartUploadTest.cpp

services/lib/src/DynamoDBTools.cpp

services/lib/src/GlobalTools.h

services/lib/src/GlobalTools.cpp

services/tunnelbroker/src/Amqp/AmqpManager.cpp

services/tunnelbroker/src/Database/DatabaseManager.cpp

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp

services/tunnelbroker/src/Tools/Tools.h

services/tunnelbroker/src/Tools/Tools.cpp

services/tunnelbroker/test/AmqpManagerTest.cpp

services/tunnelbroker/test/DatabaseManagerTest.cpp

services/tunnelbroker/test/DeliveryBrokerTest.cpp

services/tunnelbroker/test/ToolsTest.cpp

[services] Move tools to their namespaceClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 12633

services/backup/src/Constants.h

services/backup/src/DatabaseManager.cpp

services/backup/src/Reactors/server/CreateNewBackupReactor.cpp

services/backup/src/Reactors/server/SendLogReactor.cpp

services/backup/src/Tools.h

services/backup/src/Tools.cpp

services/backup/test/DatabaseManagerTest.cpp

services/blob/src/AwsS3Bucket.cpp

services/blob/src/BlobServiceImpl.cpp

services/blob/src/Constants.h

services/blob/src/DatabaseManager.cpp

services/blob/src/Reactors/server/GetReactor.h

services/blob/src/Reactors/server/PutReactor.h

services/blob/src/S3Tools.cpp

services/blob/src/Tools.h

services/blob/src/Tools.cpp

services/blob/test/MultiPartUploadTest.cpp

services/lib/src/DynamoDBTools.cpp

services/lib/src/GlobalTools.h

services/lib/src/GlobalTools.cpp

services/tunnelbroker/src/Amqp/AmqpManager.cpp

services/tunnelbroker/src/Database/DatabaseManager.cpp

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp

services/tunnelbroker/src/Tools/Tools.h

services/tunnelbroker/src/Tools/Tools.cpp

services/tunnelbroker/test/AmqpManagerTest.cpp

services/tunnelbroker/test/DatabaseManagerTest.cpp

services/tunnelbroker/test/DeliveryBrokerTest.cpp

services/tunnelbroker/test/ToolsTest.cpp

[services] Move tools to their namespace
ClosedPublic
Actions

Revision Contents
Changeset List