fgerlits commented on a change in pull request #1284:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1284#discussion_r832014350
##########
File path: libminifi/src/FlowController.cpp
##########
@@ -438,6 +440,20 @@ std::shared_ptr<state::response::ResponseNode>
FlowController::getAgentManifest(
return agentInfo;
}
+void
FlowController::executeOnAllComponents(std::function<void(state::StateController*)>
func) {
+ std::lock_guard<std::recursive_mutex> lock(mutex_);
+ for (auto* component: getAllComponents()) {
+ func(component);
+ }
+}
+
+void FlowController::executeOnComponent(const std::string &name,
std::function<void(state::StateController*)> func) {
+ std::lock_guard<std::recursive_mutex> lock(mutex_);
+ if (auto* component = getComponent(name); component != nullptr) {
+ func(component);
+ }
Review comment:
maybe add an error log if the component is not found? I think that
should never happen, and a log could be useful if it does
##########
File path: libminifi/src/FlowController.cpp
##########
@@ -523,14 +537,14 @@ void
FlowController::getAllProcessorControllers(std::vector<state::StateControll
}
}
-void FlowController::getProcessorController(const std::string& name,
std::vector<state::StateController*>& controllerVec,
+void FlowController::getProcessorController(const std::string& name,
state::StateController*& controller,
const
std::function<std::unique_ptr<state::ProcessorController>(core::Processor&)>&
controllerFactory) {
auto* processor = root_->findProcessorByName(name);
- auto& controller = processor_to_controller_[processor->getUUID()];
- if (!controller) {
- controller = controllerFactory(*processor);
+ auto& foundController = processor_to_controller_[processor->getUUID()];
Review comment:
we should check if `processor` is null before dereferencing it
##########
File path: libminifi/include/FlowController.h
##########
@@ -234,7 +233,11 @@ class FlowController : public
core::controller::ForwardingControllerServiceProvi
std::chrono::steady_clock::time_point start_time_;
private:
- void getProcessorController(const std::string& name,
std::vector<state::StateController*>& controllerVec,
+ std::vector<state::StateController*> getAllComponents();
+
+ state::StateController* getComponent(const std::string &name);
+
+ void getProcessorController(const std::string& name,
state::StateController*& controller,
const
std::function<std::unique_ptr<state::ProcessorController>(core::Processor&)>&
controllerFactory);
Review comment:
this could return the `controller` instead of using the slightly
strange-looking `state::StateController*&` out parameter:
```suggestion
state::StateController* getProcessorController(const std::string& name,
const
std::function<std::unique_ptr<state::ProcessorController>(core::Processor&)>&
controllerFactory);
```
##########
File path: libminifi/src/FlowController.cpp
##########
@@ -523,14 +537,14 @@ void
FlowController::getAllProcessorControllers(std::vector<state::StateControll
}
}
-void FlowController::getProcessorController(const std::string& name,
std::vector<state::StateController*>& controllerVec,
+void FlowController::getProcessorController(const std::string& name,
state::StateController*& controller,
const
std::function<std::unique_ptr<state::ProcessorController>(core::Processor&)>&
controllerFactory) {
auto* processor = root_->findProcessorByName(name);
- auto& controller = processor_to_controller_[processor->getUUID()];
- if (!controller) {
- controller = controllerFactory(*processor);
+ auto& foundController = processor_to_controller_[processor->getUUID()];
+ if (!foundController) {
+ foundController = controllerFactory(*processor);
}
- controllerVec.push_back(controller.get());
+ controller = foundController.get();
Review comment:
`controller` is dangling: `controllerFactory` returns a `unique_ptr`
which gets destroyed at the end of the function.
I think we should add the newly created `ProcessorController` to the
`processor_to_controller_` map, both here and in `getAllProcessorControllers`.
##########
File path: libminifi/src/c2/ControllerSocketProtocol.cpp
##########
@@ -180,14 +178,18 @@ void
ControllerSocketProtocol::initialize(core::controller::ControllerServicePro
resp.write(response.str());
stream->write(resp.getBuffer());
} else if (what == "components") {
+ std::vector<std::pair<std::string, bool>> components;
+
update_sink_->executeOnAllComponents([&components](state::StateController*
component){
+ components.emplace_back(component->getComponentName(),
component->isRunning());
+ });
io::BufferStream resp;
resp.write(&head, 1);
- const auto size_ =
gsl::narrow<uint16_t>(update_sink_->getAllComponents().size());
- resp.write(size_);
- for (const auto &component : update_sink_->getAllComponents()) {
- resp.write(component->getComponentName());
- resp.write(component->isRunning() ? "true" : "false");
+
resp.write(gsl::narrow<uint16_t>(gsl::narrow<uint16_t>(components.size())));
Review comment:
one `gsl_narrow` is enough :)
```suggestion
resp.write(gsl::narrow<uint16_t>(components.size()));
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
