I'm not sure if Communication name is ok... it's not obvious why the client is moved from client directory to Communication. Also, Communication sounds like a place where e.g. reactors could live.
If we're planning to have both clients and servers here, then it might be fine. But if only the clients, then maybe just /src/client?

This revision now requires changes to proceed.Mar 31 2022, 9:52 AM

• karol edited the summary of this revision. (Show Details)Apr 4 2022, 8:07 AM

• karol added a parent revision: D3612: [services] Backup - Apply reading log chunk in send log reactor.

In D3532#97908, @palys-swm wrote:

Shouldn't we also update the imports?

CMake seems to handle them properly.

I'm not sure if Communication name is ok... it's not obvious why the client is moved from client directory to Communication. Also, Communication sounds like a place where e.g. reactors could live.
If we're planning to have both clients and servers here, then it might be fine. But if only the clients, then maybe just /src/client?

The idea was to store the reactor classes in the Reactors directory. But ServiceBlobClient is not a reactor class so I wanted to keep it in a separate location. src/client could work, but maybe something more descriptive, like src/grpc-client? So there's also an information for what that client will be used?

I'm not sure if Communication name is ok... it's not obvious why the client is moved from client directory to Communication. Also, Communication sounds like a place where e.g. reactors could live.
If we're planning to have both clients and servers here, then it might be fine. But if only the clients, then maybe just /src/client?

The idea was to store the reactor classes in the Reactors directory. But ServiceBlobClient is not a reactor class so I wanted to keep it in a separate location. src/client could work, but maybe something more descriptive, like src/grpc-client? So there's also an information for what that client will be used?

Good idea! src/grpc-client will work well

This revision now requires changes to proceed.Apr 5 2022, 9:23 AM

changed dir name as discussed

Harbormaster completed remote builds in B7907: Diff 11086.Apr 6 2022, 12:45 AM

• jim added inline comments.Apr 6 2022, 5:49 AM

services/backup/docker-server/contents/server/CMakeLists.txt
52 ↗	(On Diff #11086)	Why do you need to put all these subdirectories in the include path? This is much more confusing than including with the path relative to `src`, like `#include "grpc-client/whatever.h"`.

• karol added inline comments.Apr 6 2022, 6:12 AM

services/backup/docker-server/contents/server/CMakeLists.txt
52 ↗	(On Diff #11086)	For me, it is better. I guess there would be a problem if we wanted to have two files of the same name in separate directories. But otherwise, what are the downsides of having includes with file names only? Ok, I have to add the paths to CMake, but still, if I don't I can immediately see an error so I know what went wrong. Such an approach avoids "include hell". It's all good if you include in the file that's in the root dir, but if you include in the file that is in the subdir from another file that is in another subdir, you end up with something like this: #include "../../../Reactors/client/base-reactors/ClientBidiReactorBase.h" For me it looks/works better to do it like this: #include "ClientBidiReactorBase.h" It's more readable, easier to maintain and easier to refactor if necessary.