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<state::response::SerializedResponseNode> metrics;
+    std::shared_ptr<state::response::NodeReporter> reporter;
+    std::shared_ptr<state::response::ResponseNode> 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.
   
   I see two main root causes for this:
    - C2Agent sometimes needs a different set of information, but the 
information is only accessible through a composite object (`AgentInformation` 
or `AgentInformationWithoutManifest`) which contains a hardcoded set of 
information
    - we have no method of communicating the needs of the `C2Agent` to 
`AgentInformation`, therefore we can't request a particular set of information 
(in this case `C2Agent` would specify the set of information needed), nor do we 
have the ability to give feedback about the need for including a particular set 
of information (in this case `AgentInformation` would be able to decide what 
information to include based on the state of `C2Agent`, namely whether the 
first heartbeat has been successfully sent)
   
    If we can find no better solution I can live with this until we change the 
architecture, but we will have to consider how to refactor the C2 architecture 
to better fit our needs.

----------------------------------------------------------------
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

Reply via email to