[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement lightweight C2 heartbeat.
bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement
lightweight C2 heartbeat.
URL: https://github.com/apache/nifi-minifi-cpp/pull/734#discussion_r385009945
##
File path: extensions/http-curl/tests/HTTPHandlers.h
##
@@ -343,4 +345,104 @@ class DeleteTransactionResponder : public CivetHandler {
std::string response_code;
};
+class HeartbeatHandler : public CivetHandler {
Review comment:
I like this!
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement lightweight C2 heartbeat.
bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement
lightweight C2 heartbeat.
URL: https://github.com/apache/nifi-minifi-cpp/pull/734#discussion_r385037599
##
File path: conf/minifi.properties
##
@@ -51,7 +51,7 @@
nifi.database.content.repository.directory.default=${MINIFI_HOME}/content_reposi
#nifi.c2.flow.base.url=
#nifi.c2.rest.url=
#nifi.c2.rest.url.ack=
-nifi.c2.root.classes=DeviceInfoNode,AgentInformation,FlowInformation
+nifi.c2.root.classes=DeviceInfoNode,AgentInformationWithoutManifest,FlowInformation
Review comment:
I think changing this by default could cause problems.
If someone wants to use an older C2 server with newer agents without
adapting the C2 server to send DESCRIBE manifest requests when needed would
have their workflow broken.
I think the possibility of changing to lightweight heartbeats should be
documented in `C2.md`, and even a commented out line here with a
`nifi.c2.root.classes` containing `AgentInformationWithoutManifest` (and an
explanation of what it does) would be fine, but changing it by default does not
seem safe.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement lightweight C2 heartbeat.
bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement
lightweight C2 heartbeat.
URL: https://github.com/apache/nifi-minifi-cpp/pull/734#discussion_r384998966
##
File path: libminifi/include/core/state/nodes/AgentInformation.h
##
@@ -80,7 +80,7 @@ class ComponentManifest : public DeviceInformation {
return CoreComponent::getName();
}
- virtual std::vector serialize() {
+ virtual std::vector serialize() const {
Review comment:
While I agree with this modification, it breaks API, which we can't do in a
minor release.
Anyone's code who has implemented a ResponseNode would fail to compile with
this change.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement lightweight C2 heartbeat.
bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement
lightweight C2 heartbeat.
URL: https://github.com/apache/nifi-minifi-cpp/pull/734#discussion_r385035262
##
File path: extensions/http-curl/tests/C2JstackTest.cpp
##
@@ -45,123 +45,114 @@
#include "CivetServer.h"
#include
#include "protocols/RESTSender.h"
+#include "HTTPIntegrationBase.h"
+#include "HTTPHandlers.h"
-void waitToVerifyProcessor() {
- std::this_thread::sleep_for(std::chrono::seconds(10));
-}
+class VerifyC2Describe : public VerifyC2Base {
+ public:
+ explicit VerifyC2Describe(bool isSecure)
+ : VerifyC2Base(isSecure) {
+ }
+ virtual ~VerifyC2Describe() = default;
-class ConfigHandler : public CivetHandler {
- public:
- ConfigHandler() {
-calls_ = 0;
+ void testSetup() {
+LogTestController::getInstance().setTrace();
+LogTestController::getInstance().setDebug();
+LogTestController::getInstance().setInfo();
+VerifyC2Base::testSetup();
}
- bool handlePost(CivetServer *server, struct mg_connection *conn) {
-calls_++;
-std::string heartbeat_response = "{\"operation\" :
\"heartbeat\",\"requested_operations\": [ {"
- "\"operation\" : \"describe\", "
- "\"operationid\" : \"8675309\", "
- "\"name\": \"jstack\""
- "}]}";
- mg_printf(conn, "HTTP/1.1 200 OK\r\nContent-Type: "
-"text/plain\r\nContent-Length: %lu\r\nConnection:
close\r\n\r\n",
-heartbeat_response.length());
- mg_printf(conn, "%s", heartbeat_response.c_str());
-
-
-return true;
+
+ void configureC2RootClasses() {
+configuration->set("nifi.c2.root.classes",
"DeviceInfoNode,AgentInformationWithoutManifest,FlowInformation");
}
+};
- bool handleGet(CivetServer *server, struct mg_connection *conn) {
-std::ifstream myfile(test_file_location_.c_str());
-
-if (myfile.is_open()) {
- std::stringstream buffer;
- buffer << myfile.rdbuf();
- std::string str = buffer.str();
- myfile.close();
- mg_printf(conn, "HTTP/1.1 200 OK\r\nContent-Type: "
-"text/plain\r\nContent-Length: %lu\r\nConnection:
close\r\n\r\n",
-str.length());
- mg_printf(conn, "%s", str.c_str());
-} else {
- mg_printf(conn, "HTTP/1.1 500 Internal Server Error\r\n");
-}
+class VerifyC2DescribeJstack : public VerifyC2Describe {
+ public:
+ explicit VerifyC2DescribeJstack(bool isSecure)
+ : VerifyC2Describe(isSecure) {
+ }
+
+ virtual ~VerifyC2DescribeJstack() = default;
-return true;
+ virtual void runAssertions() {
+assert(LogTestController::getInstance().contains("SchedulingAgent") ==
true);
}
- std::string test_file_location_;
- std::atomic calls_;
+
};
-int main(int argc, char **argv) {
- mg_init_library(0);
- LogTestController::getInstance().setInfo();
- LogTestController::getInstance().setDebug();
- LogTestController::getInstance().setDebug();
- LogTestController::getInstance().setTrace();
-
- const char *options[] = { "document_root", ".", "listening_ports", "0", 0 };
- std::vector cpp_options;
- for (int i = 0; i < (sizeof(options) / sizeof(options[0]) - 1); i++) {
-cpp_options.push_back(options[i]);
+class DescribeManifestHandler: public HeartbeatHandler {
Review comment:
It is misleading to have both the DESCRIBE manifest and DESCRIBE jstack
tests in a test named C2JstackTest. I would prefer if these were in two
separate files (and therefore two separate tests).
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement lightweight C2 heartbeat.
bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement
lightweight C2 heartbeat.
URL: https://github.com/apache/nifi-minifi-cpp/pull/734#discussion_r380131132
##
File path: libminifi/src/c2/C2Agent.cpp
##
@@ -315,16 +317,35 @@ void C2Agent::performHeartBeat() {
payload.addPayload(std::move(deviceInfo));
}
- if (!root_response_nodes_.empty()) {
-for (auto metric : root_response_nodes_) {
- C2Payload child_metric_payload(Operation::HEARTBEAT);
- child_metric_payload.setLabel(metric.first);
- if (metric.second->isArray()) {
-child_metric_payload.setContainer(true);
- }
- serializeMetrics(child_metric_payload, metric.first,
metric.second->serialize(), metric.second->isArray());
- payload.addPayload(std::move(child_metric_payload));
+ for (auto metric : root_response_nodes_) {
+C2Payload child_metric_payload(Operation::HEARTBEAT);
+bool isArray{false};
+std::string metricName;
+std::vector metrics;
+std::shared_ptr reporter;
+std::shared_ptr agentInfoManifest;
+
+//Send agent manifest in first heartbeat
Review comment:
I like having the manifest in the first heartbeat, I think it makes sense.
I dislike how we had to implement it, but if we create follow-up tickets for
this to figure out how to refactor the architecture, I am OK with it for the
time being.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement lightweight C2 heartbeat.
bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement
lightweight C2 heartbeat.
URL: https://github.com/apache/nifi-minifi-cpp/pull/734#discussion_r379521138
##
File path: libminifi/src/c2/C2Agent.cpp
##
@@ -506,13 +527,16 @@ void C2Agent::handle_describe(const C2ContentResponse
&resp) {
}
std::vector> metrics_vec;
-
- reporter->getResponseNodes(metrics_vec, metric_class_id);
C2Payload response(Operation::ACKNOWLEDGE, resp.ident, false, true);
response.setLabel("metrics");
+
+ C2Payload metrics(Operation::ACKNOWLEDGE, resp.ident, false, true);
Review comment:
Sounds fair, I am OK with this with a follow-up Jira, thank you.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement lightweight C2 heartbeat.
bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement
lightweight C2 heartbeat.
URL: https://github.com/apache/nifi-minifi-cpp/pull/734#discussion_r379520331
##
File path: libminifi/src/c2/C2Agent.cpp
##
@@ -315,16 +317,35 @@ void C2Agent::performHeartBeat() {
payload.addPayload(std::move(deviceInfo));
}
- if (!root_response_nodes_.empty()) {
-for (auto metric : root_response_nodes_) {
- C2Payload child_metric_payload(Operation::HEARTBEAT);
- child_metric_payload.setLabel(metric.first);
- if (metric.second->isArray()) {
-child_metric_payload.setContainer(true);
- }
- serializeMetrics(child_metric_payload, metric.first,
metric.second->serialize(), metric.second->isArray());
- payload.addPayload(std::move(child_metric_payload));
+ for (auto metric : root_response_nodes_) {
+C2Payload child_metric_payload(Operation::HEARTBEAT);
+bool isArray{false};
+std::string metricName;
+std::vector metrics;
+std::shared_ptr reporter;
+std::shared_ptr agentInfoManifest;
+
+//Send agent manifest in first heartbeat
Review comment:
> May be the function "getAgentInformationWithManifest" name is confusing to
you. If you look at this function, it instantiates "AgentInformation" class.
Will change the name to "getAgentInformation" instead.
Yep, it did confuse me, thanks for the clarification.
Let me unpack the other parts of why this does not feel right (while
describing my understanding of the processes behind our C2 implementation).
The classes we want to use for C2 responses are defined in the
`nifi.c2.root.classes` property.
These are then instantiated using our classloader architecture by
`FlowController::initializeC2`, static casted into
`state::response::ResponseNode`s and stored in root_response_nodes_ .
3 checks are made on these objects using dynamic casts:
- if they implement `state::response::AgentIdentifier`, the agent
identifier and class are set on them
- if they implement `state::response::AgentMonitor`, the provenance and
flowfile repos and the `FlowController` itself are made known to them
- if they implement `state::response::FlowMonitor` flow-related information
and the `FlowController` itself is made known to them
We don't assume anything about these classes other than based on what
interfaces they implement and we only interact with them through these
interfaces.
These response nodes are exposed through `FlowController::getResponseNodes`
(which overrides `StateManager::getResponseNodes`), and get fed into the
C2Agent through a convoluted scheme involving registering the C2Agent as an
`UpdateController` via `registerUpdateListener` and a TreeUpdateListener
through which the `FlowController::getResponseNodes()` is looped back into
`FlowController::setResponseNodes` (which in reality is
`StateManager::setResponseNodes`, because `FlowController` does not override
that function), which will call `setResponseNodes` on all registered
UpdateControllers, thereby calling `C2Agent::setResponseNodes`, completing a
section of the Rube Goldberg machine that we call our C2 implementation.
The only thing `C2Agent` did so far was to call `serializeMetrics` on these
when needed by `performHeartBeat`. `serializeMetrics` depends only on the
interfaces implemented by the defined response classes: all behaviour is
defined by them.
What your change does, is if the CoreComponent name of the one of these
classes match `agentInfo` (which is the name of both
`AgentInformationWithoutManifest` and `AgentInformation`) overrides the
behaviour of an class defined by the user in `nifi.c2.root.classes` and
instantiated by `FlowController` by creating a new instance of
`AgentInformation` via `FlowController::getAgentInformation` and using that for
the first heartbeat irrespective of what the configured class is.
This, instead of depending on interfaces and letting the behaviour stay at
level of the response classes, changes behaviour at the level of the `C2Agent`,
based on the name of the response node instance, something which we did not
depend on so far. It also makes us expose a hardcoded object factory on
`FlowController` for the `AgentInformation` class
(`FlowController::getAgentInformation()`); something that was not needed so
far, as everything was done dynamically through our classloader.
I do not have a perfect solution for this, and I do not know if a perfect
solution exists in the current architecture.
We could change`AgentInformationWithoutManifest` to include the manifest on
its first `serialize` call, but that would not be a solution: we cannot be sure
and we should not depend on whether that call was done by the C2Agent and
whether the resulting heartbeat was successfully delivered, thereby really
negating the need to include the manifest in further serializations.
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement lightweight C2 heartbeat.
bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement
lightweight C2 heartbeat.
URL: https://github.com/apache/nifi-minifi-cpp/pull/734#discussion_r379520331
##
File path: libminifi/src/c2/C2Agent.cpp
##
@@ -315,16 +317,35 @@ void C2Agent::performHeartBeat() {
payload.addPayload(std::move(deviceInfo));
}
- if (!root_response_nodes_.empty()) {
-for (auto metric : root_response_nodes_) {
- C2Payload child_metric_payload(Operation::HEARTBEAT);
- child_metric_payload.setLabel(metric.first);
- if (metric.second->isArray()) {
-child_metric_payload.setContainer(true);
- }
- serializeMetrics(child_metric_payload, metric.first,
metric.second->serialize(), metric.second->isArray());
- payload.addPayload(std::move(child_metric_payload));
+ for (auto metric : root_response_nodes_) {
+C2Payload child_metric_payload(Operation::HEARTBEAT);
+bool isArray{false};
+std::string metricName;
+std::vector metrics;
+std::shared_ptr reporter;
+std::shared_ptr agentInfoManifest;
+
+//Send agent manifest in first heartbeat
Review comment:
> May be the function "getAgentInformationWithManifest" name is confusing to
you. If you look at this function, it instantiates "AgentInformation" class.
Will change the name to "getAgentInformation" instead.
Yep, it did confuse me, thanks for the clarification.
Let me unpack the other parts of why this does not feel right (while
describing my understanding of the processes behind our C2 implementation).
The classes we want to use for C2 responses are defined in the
`nifi.c2.root.classes` property.
These are then instantiated using our classloader architecture by
`FlowController::initializeC2`, static casted into
`state::response::ResponseNode`s and stored in root_response_nodes_ .
3 checks are made on these objects using dynamic casts:
- if they implement `state::response::AgentIdentifier`, the agent
identifier and class are set on them
- if they implement `state::response::AgentMonitor`, the provenance and
flowfile repos and the `FlowController` itself are made known to them
- if they implement `state::response::FlowMonitor` flow-related information
and the `FlowController` itself is made known to them
We don't assume anything about these classes other than based on what
interfaces they implement and we only interact with them through these
interfaces.
These response nodes are exposed through `FlowController::getResponseNodes`
(which overrides `StateManager::getResponseNodes`), and get fed into the
C2Agent through a convoluted scheme involving registering the C2Agent as an
`UpdateController` via `registerUpdateListener` and a TreeUpdateListener
through which the `FlowController::getResponseNodes()` is looped back into
`FlowController::setResponseNodes` (which in reality is
`StateManager::setResponseNodes`, because `FlowController` does not override
that function), which will call `setResponseNodes` on all registered
UpdateControllers, thereby calling `C2Agent::setResponseNodes`, completing a
section of the Rube Goldberg machine that we call our C2 implementation.
The only thing `C2Agent` did so far was to call `serializeMetrics` on these
when needed by `performHeartBeat`. `serializeMetrics` depends only on the
interfaces implemented by the defined response classes: all behaviour is
defined by them.
What your change does, is if the CoreComponent name of the one of these
classes match `agentInfo` (which is the name of both
`AgentInformationWithoutManifest` and `AgentInformation`) overrides the
behaviour of an class defined by the user in `nifi.c2.root.classes` and
instantiated by `FlowController` by creating a new instance of
`AgentInformation` via `FlowController::getAgentInformation` and using that for
the first heartbeat irrespective of what the configured class is.
This, instead of depending on interfaces and letting the behaviour stay at
level of the response classes, changes behaviour at the level of the `C2Agent`,
based on the name of the response node instance, something which we did not
depend on so far. It also makes us expose a hardcoded object factory on
`FlowController` for the `AgentInformation` class
(`FlowController::getAgentInformation()`); something that was not needed so
far, as everything was done dynamically through our classloader.
I do not have a perfect solution for this, and I do not know if a perfect
solution exists in the current architecture.
We could change`AgentInformationWithoutManifest` to include the manifest on
its first `serialize` call, but that would not be a solution: we cannot be sure
and we should not depend on whether that call was done by the C2Agent and
whether the resulting heartbeat was successfully delivered, thereby really
negating the need to include the manifest in further serializations.
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement lightweight C2 heartbeat.
bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement
lightweight C2 heartbeat.
URL: https://github.com/apache/nifi-minifi-cpp/pull/734#discussion_r378765083
##
File path: libminifi/src/c2/C2Agent.cpp
##
@@ -553,6 +577,20 @@ void C2Agent::handle_describe(const C2ContentResponse
&resp) {
}
response.addPayload(std::move(options));
+auto reporter =
std::dynamic_pointer_cast(update_sink_);
+if (reporter != nullptr) {
+ std::vector> metrics_vec;
+
+ C2Payload agentInfo(Operation::ACKNOWLEDGE, resp.ident, false, true);
+ agentInfo.setLabel("agentInfo");
+
+ reporter->getManifestNodes(metrics_vec);
+ for (auto& metric : metrics_vec) {
+ serializeMetrics(agentInfo, metric->getName(), metric->serialize());
+ }
+ response.addPayload(std::move(agentInfo));
+}
+
if (device_information_.size() > 0) {
Review comment:
I know this is not new code, but I don't see any place where `C2Agent`'s
`device_information_` would get filled, making this dead code (along with its
counterpart in `performHeartBeat`).
Device information seems to be sourced and transmitted from the
`DeviceInfoNode` as a "deviceInfo" root node anyway, and not as
"AgentInformation".
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement lightweight C2 heartbeat.
bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement
lightweight C2 heartbeat.
URL: https://github.com/apache/nifi-minifi-cpp/pull/734#discussion_r378754450
##
File path: libminifi/src/c2/C2Agent.cpp
##
@@ -539,9 +563,9 @@ void C2Agent::handle_describe(const C2ContentResponse
&resp) {
} else if (resp.name == "manifest") {
auto keys = configuration_->getConfiguredKeys();
C2Payload response(Operation::ACKNOWLEDGE, resp.ident, false, true);
-response.setLabel("configuration_options");
+response.setLabel("configurationOptions");
Review comment:
"jstack" also utilizes this label, although unchanged (still
"configuration_options"), but fills it with different information. This does
not look right.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement lightweight C2 heartbeat.
bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement
lightweight C2 heartbeat.
URL: https://github.com/apache/nifi-minifi-cpp/pull/734#discussion_r378754450
##
File path: libminifi/src/c2/C2Agent.cpp
##
@@ -539,9 +563,9 @@ void C2Agent::handle_describe(const C2ContentResponse
&resp) {
} else if (resp.name == "manifest") {
auto keys = configuration_->getConfiguredKeys();
C2Payload response(Operation::ACKNOWLEDGE, resp.ident, false, true);
-response.setLabel("configuration_options");
+response.setLabel("configurationOptions");
Review comment:
Why does this have the same label as the response for "configuration"?
It would also have the same label as the response for "jstack", but "jstack"
hasn't been changed to "configurationOptions".
This does not look right.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement lightweight C2 heartbeat.
bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement
lightweight C2 heartbeat.
URL: https://github.com/apache/nifi-minifi-cpp/pull/734#discussion_r378752297
##
File path: libminifi/src/c2/C2Agent.cpp
##
@@ -506,13 +527,16 @@ void C2Agent::handle_describe(const C2ContentResponse
&resp) {
}
std::vector> metrics_vec;
-
- reporter->getResponseNodes(metrics_vec, metric_class_id);
C2Payload response(Operation::ACKNOWLEDGE, resp.ident, false, true);
response.setLabel("metrics");
+
+ C2Payload metrics(Operation::ACKNOWLEDGE, resp.ident, false, true);
Review comment:
Why are two layers of `C2Payload` needed with the same label? "jstack" has
only one to which the children containing the real data is added.
"configuration" is rewritten here to have a "configurationOptions" node to
which an other "configurationOptions" node is added to which the children are
added.
"manifest" is written with the same extra layer.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement lightweight C2 heartbeat.
bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement
lightweight C2 heartbeat.
URL: https://github.com/apache/nifi-minifi-cpp/pull/734#discussion_r378755472
##
File path: libminifi/src/c2/C2Agent.cpp
##
@@ -539,9 +563,9 @@ void C2Agent::handle_describe(const C2ContentResponse
&resp) {
} else if (resp.name == "manifest") {
auto keys = configuration_->getConfiguredKeys();
Review comment:
This, unlike the "configuration" branch does not sanitize the keys. Isn't
this an issue?
(Also, if this is this way because of a mistake, removing the code
duplication between the two should prevent such things happening.)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement lightweight C2 heartbeat.
bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement
lightweight C2 heartbeat.
URL: https://github.com/apache/nifi-minifi-cpp/pull/734#discussion_r378360255
##
File path: libminifi/src/c2/C2Agent.cpp
##
@@ -315,16 +317,35 @@ void C2Agent::performHeartBeat() {
payload.addPayload(std::move(deviceInfo));
}
- if (!root_response_nodes_.empty()) {
-for (auto metric : root_response_nodes_) {
- C2Payload child_metric_payload(Operation::HEARTBEAT);
- child_metric_payload.setLabel(metric.first);
- if (metric.second->isArray()) {
-child_metric_payload.setContainer(true);
- }
- serializeMetrics(child_metric_payload, metric.first,
metric.second->serialize(), metric.second->isArray());
- payload.addPayload(std::move(child_metric_payload));
+ for (auto metric : root_response_nodes_) {
+C2Payload child_metric_payload(Operation::HEARTBEAT);
+bool isArray{false};
+std::string metricName;
+std::vector metrics;
+std::shared_ptr reporter;
+std::shared_ptr agentInfoManifest;
+
+//Send agent manifest in first heartbeat
Review comment:
This feels very much tacked on.
If I understand correctly, the purpose is to send a manifest in the first
heartbeat, irrespective of what "flavour" of `AgentInformation` is configured,
which is detected by all of them having the name of `agentManifest`, and
implemented by using an instance of the newly created
`AgentInformationWithManifest`.
This feels like it violates the layering of this whole concept by
intercepting response nodes by name and changing, and results in adding the
`AgentInformationWithManifest` class and using it even when not
`AgentInformationWithoutManifest` but `AgentInformation` is configured, when it
is unwarranted.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement lightweight C2 heartbeat.
bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement
lightweight C2 heartbeat.
URL: https://github.com/apache/nifi-minifi-cpp/pull/734#discussion_r378749471
##
File path: libminifi/src/c2/C2Agent.cpp
##
@@ -521,9 +545,9 @@ void C2Agent::handle_describe(const C2ContentResponse
&resp) {
std::vector keys;
std::copy_if(unsanitized_keys.begin(), unsanitized_keys.end(),
std::back_inserter(keys), [](std::string key) {return key.find("pass") ==
std::string::npos;});
C2Payload response(Operation::ACKNOWLEDGE, resp.ident, false, true);
-response.setLabel("configuration_options");
+response.setLabel("configurationOptions");
Review comment:
Why was this changed? Doesn't this break backward compatibility? (I guess if
DESCRIBE has not been formalized yet it doesn't, but I want to make sure.)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
