[#175] Client program that spawn other clients #242

marcocapozzoli · 2025-02-03T09:21:20Z

No description provided.

angeloprobst · 2025-02-03T11:54:16Z

src/cpp/client_spawner/SentinelServerNode.cc

+    shared_ptr<Message> message = StarNode::message_factory(command, args);
+    if (message) {
+        return message;
+    }
+    if (command == WORKER_NOTIFICATION) {
+        return shared_ptr<Message>(new WorkerMessage(args[0], args[1]));
+    }
+    return shared_ptr<Message>{};


This is not a change request, it's more like a tip.
The following code does the same, but it's a bit simpler and complies to the Modern C++ style recommendations.

auto message = StarNode::message_factory(command, args); // <== here if (message) { return message; } if (command == WORKER_NOTIFICATION) { return make_shared<WorkerMessage>(args[0], args[1]); // <== here } return nullptr; // <== here

angeloprobst · 2025-02-03T12:17:44Z

src/cpp/client_spawner/SentinelServerNode.h

+
+#include "StarNode.h"
+
+#define DEBUG


This is hard-coded but DEBUG is supposed to be set only while running the build process by the building tool, in our case, Bazel. By default, Bazel does not set -DNDEBUG (see here), which means that DEBUG is always enabled unless we instruct Bazel to not doing it.

The way we are currently running the build process does not allow us to pass options to Bazel, but, once we consider our services stable and ready for production, the build process will be adjusted in order to allow non-debug builds.

In short, you can, and should, remove this #define DEBUG.

angeloprobst · 2025-02-03T12:20:07Z

src/cpp/client_spawner/WorkerClientNode.h

+
+    ~WorkerClientNode();
+
+    void execute(vector<string>& request);


Suggested change

void execute(vector<string>& request);

void execute(const vector<string>& request);

angeloprobst · 2025-02-03T12:20:29Z

src/cpp/client_spawner/WorkerClientNode.cc

+
+WorkerClientNode::~WorkerClientNode() {}
+
+void WorkerClientNode::execute(vector<string> &request) {


Suggested change

void WorkerClientNode::execute(vector<string> &request) {

void WorkerClientNode::execute(const vector<string> &request) {

angeloprobst · 2025-02-03T12:23:21Z

src/cpp/client_spawner/WorkerClientNode.cc

+        cout << no_match << endl;
+        message.push_back(no_match);
+    } else {
+        cout << query_answer_str << endl;


Suggested change

cout << no_match << endl;

message.push_back(no_match);

} else {

cout << query_answer_str << endl;

#ifdef DEBUG

cout << no_match << endl;

#endif

message.push_back(no_match);

} else {

#ifdef DEBUG

cout << query_answer_str << endl;

#endif

angeloprobst · 2025-02-03T12:36:06Z

Makefile

@@ -28,6 +28,9 @@ github-runner:
 build-image:
 	cd src && bash -x scripts/docker_image_build.sh

+build-spawner-image:


See comment on src/scripts/docker_image_spawner_build.sh file.

Suggested change

build-spawner-image:

build-spawner-image: build

angeloprobst · 2025-02-03T12:37:05Z

src/scripts/docker_image_spawner_build.sh

@@ -0,0 +1,28 @@
+#!/bin/bash -x


I think the dependencies in this script should be replaced by dependencies of a target in the Makefile.

I mean, as the building image (L9) and the build process (L16) already have target entries in the Makefile, this script should only run the docker buildx ... command (L28) for the spawner and have its own Makefile target entry with its deps set accordingly.

angeloprobst · 2025-02-03T12:43:02Z

src/cpp/client_spawner/SentinelServerNode.h

+
+    ~SentinelServerNode();
+
+    void storage_message(string &worker_id, string &message);


Maybe?

Suggested change

void storage_message(string &worker_id, string &message);

void storage_message(const string &worker_id, const string &message);

Please set them const if the strings are not supposed to be changed inside the function.

angeloprobst · 2025-02-03T12:44:16Z

src/cpp/client_spawner/SentinelServerNode.h

+public:
+    string worker_id;
+    string msg;
+    WorkerMessage(string &worker_id, string &msg);


Suggested change

WorkerMessage(string &worker_id, string &msg);

WorkerMessage(const string &worker_id, const string &msg);

angeloprobst · 2025-02-03T12:46:50Z

src/cpp/client_spawner/SentinelServerNode.cc

+WorkerMessage::WorkerMessage(string &worker_id, string &msg) {
+    this->worker_id = worker_id;
+    this->msg = msg;
+}


Suggested change

WorkerMessage::WorkerMessage(string &worker_id, string &msg) {

this->worker_id = worker_id;

this->msg = msg;

}

WorkerMessage::WorkerMessage(const string &worker_id, const string &msg)

: worker_id(worker_id), msg(msg) {}

angeloprobst · 2025-02-03T12:56:14Z

src/cpp/client_spawner/WorkerClientNode.cc

+WorkerClientNode::WorkerClientNode(
+    const string &node_id,
+    const string &server_id,
+    const string &das_node_server_id
+) : StarNode(node_id, server_id) {
+        this->node_id = node_id;
+        this->server_id = server_id;
+        this->das_node_server_id = das_node_server_id;
+}


StarNode is already storing server_id (attr has the same name), and DistributedAlgorithmNode (parent of StarNode) is also storing node_id as my_node_id (private) but accessible via node_id() function.

Suggested change

WorkerClientNode::WorkerClientNode(

const string &node_id,

const string &server_id,

const string &das_node_server_id

) : StarNode(node_id, server_id) {

this->node_id = node_id;

this->server_id = server_id;

this->das_node_server_id = das_node_server_id;

}

WorkerClientNode::WorkerClientNode(

const string &node_id, const string &server_id, const string &das_node_server_id

) : das_node_server_id(das_node_server_id), StarNode(node_id, server_id) {}

angeloprobst · 2025-02-03T12:56:57Z

src/cpp/client_spawner/WorkerClientNode.h

+    void execute(vector<string>& request);
+
+private:
+    string node_id;


Already defined in DistributedAlgorithNode as my_node_id (private) but accessible via node_id() function - it can removed here.

Suggested change

string node_id;

angeloprobst · 2025-02-03T12:58:26Z

src/cpp/client_spawner/WorkerClientNode.h

+
+private:
+    string node_id;
+    string das_node_server_id;


Is this any different from server_id defined in StarNode?

angeloprobst · 2025-02-03T13:02:43Z

src/cpp/client_spawner/WorkerClientNode.cc

+    vector<string> message;
+
+    message.push_back(this->node_id);


Suggested change

vector<string> message;

message.push_back(this->node_id);

vector<string> message = {this->node_id()};

andre-senna

I didn't revise C++ style as @angeloprobst and @eddiebrissow are more reliable in this regard.

But There's a design flaw in this approach which I don't think can be fixed in this PR. This flaw is not your fault, actually it's mine. The way I was envisioning the algorithm execution is not actually well coupled with our architecture. I'm already working on this.

Regarding this PR, my suggestion is to fix it as the other revisor(s) suggested but keep it UNMERGED. You will probably need pieces of this code later on.

marcocapozzoli added 4 commits February 2, 2025 16:58

create spawner script

e020010

improve Dockerfile and add log file for server

1bf9279

remove unused command

eaeb0c3

remove spawner in bazel_build

3bca48b

marcocapozzoli requested review from eddiebrissow, Pedrobc89, andre-senna and angeloprobst February 3, 2025 09:21

angeloprobst reviewed Feb 3, 2025

View reviewed changes

andre-senna requested changes Feb 3, 2025

View reviewed changes

Fixed issues based on PR review

c61e189

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#175] Client program that spawn other clients #242

[#175] Client program that spawn other clients #242

marcocapozzoli commented Feb 3, 2025

angeloprobst Feb 3, 2025

angeloprobst Feb 3, 2025 •

edited

Loading

angeloprobst Feb 3, 2025

angeloprobst Feb 3, 2025

angeloprobst Feb 3, 2025

angeloprobst Feb 3, 2025

angeloprobst Feb 3, 2025

angeloprobst Feb 3, 2025

angeloprobst Feb 3, 2025

angeloprobst Feb 3, 2025 •

edited

Loading

angeloprobst Feb 3, 2025 •

edited

Loading

angeloprobst Feb 3, 2025 •

edited

Loading

angeloprobst Feb 3, 2025

angeloprobst Feb 3, 2025 •

edited

Loading

andre-senna left a comment •

edited

Loading

	void execute(vector<string>& request);
	void execute(const vector<string>& request);


		WorkerClientNode::~WorkerClientNode() {}

		void WorkerClientNode::execute(vector<string> &request) {

	void WorkerClientNode::execute(vector<string> &request) {
	void WorkerClientNode::execute(const vector<string> &request) {


		~SentinelServerNode();

		void storage_message(string &worker_id, string &message);

	void storage_message(string &worker_id, string &message);
	void storage_message(const string &worker_id, const string &message);

	WorkerMessage(string &worker_id, string &msg);
	WorkerMessage(const string &worker_id, const string &msg);


		#include "StarNode.h"

		#define DEBUG

[#175] Client program that spawn other clients #242

Are you sure you want to change the base?

[#175] Client program that spawn other clients #242

Conversation

marcocapozzoli commented Feb 3, 2025

Choose a reason for hiding this comment

angeloprobst Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angeloprobst Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

angeloprobst Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

angeloprobst Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angeloprobst Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

andre-senna left a comment • edited Loading

Choose a reason for hiding this comment

angeloprobst Feb 3, 2025 •

edited

Loading

angeloprobst Feb 3, 2025 •

edited

Loading

angeloprobst Feb 3, 2025 •

edited

Loading

angeloprobst Feb 3, 2025 •

edited

Loading

angeloprobst Feb 3, 2025 •

edited

Loading

andre-senna left a comment •

edited

Loading