msharee9 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_r379564619
########## 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: Replying to your first part of the comment, I agree with you that the registration of C2Agent and the code involving setting and getting response nodes is not very clean and difficult to understand. From my understanding, FlowController is NodeReporter (metrics source) and C2Agent is ResponseNodeSink (metrics sink). The TreeUpdateListener runs a loop in a thread I believe which constantly updates the sink cache with metrics and I think this is unnecessary optimization. Up until now, the FlowController was responsible for instantiating classes that were defined in the nifi.c2.root.classes property, exposed via getResponseNodes and retrieved by the C2Agent when needed. The change I made also exposes the class via FlowController, this time without it being defined in the properties but that is out of necessity for optimization. (I am talking about first full heartbeat). I agree that the behavior wise this is a little hacky that we are now making a behavioral change in C2Agent rather than making that decision in the metrics classes themselves but this is because there is no way of knowing about the C2Agent state in those classes as you mentioned. We definitely need a cleaner design especially surrounding the structure of metrics classes and the source and sink concepts. I believe may be a visitor approach could make this design much simpler. Having said that, we can still remove the first heartbeat optimization in lieu of putting back the behavior in the metrics classes whichever is a better tradeoff. What do you think? ---------------------------------------------------------------- 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