fgerlits commented on a change in pull request #1045:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1045#discussion_r612369176
##########
File path: libminifi/include/core/ProcessGroup.h
##########
@@ -57,7 +60,12 @@ enum ProcessGroupType {
// ProcessGroup Class
class ProcessGroup : public CoreComponent {
+ friend struct ::TestAccessor;
Review comment:
`TestAccessor` could have a more specific name like
`ProcessGroupTestAccessor`, especially since it is in the global namespace
##########
File path: libminifi/src/core/FlowConfiguration.cpp
##########
@@ -116,6 +116,10 @@ std::unique_ptr<core::ProcessGroup>
FlowConfiguration::createRootProcessGroup(st
return std::unique_ptr<core::ProcessGroup>(new
core::ProcessGroup(core::ROOT_PROCESS_GROUP, name, uuid, version));
}
+std::unique_ptr<core::ProcessGroup>
FlowConfiguration::createSimpleProcessGroup(std::string name, utils::Identifier
& uuid, int version) {
Review comment:
both `name` and `uuid` should be `const&`
##########
File path: libminifi/src/core/ProcessGroup.cpp
##########
@@ -255,21 +245,21 @@ void ProcessGroup::stopProcessing(const
std::shared_ptr<TimerDrivenSchedulingAge
}
}
-std::shared_ptr<Processor> ProcessGroup::findProcessorById(const
utils::Identifier& uuid) const {
+std::shared_ptr<Processor> ProcessGroup::findProcessorById(const
utils::Identifier& uuid, Traverse traverse) const {
const auto id_matches = [&] (const std::shared_ptr<Processor>& processor) {
logger_->log_debug("Current processor is %s", processor->getName());
Review comment:
This is old code, but I would demote this to trace level, or make the
log message more specific (eg. "Searching for processor by ID, checking
processor %s"), or both. Another option would be to get rid of this log line
completely.
(Similarly in `findProcessorByName` and `getAllProcessors`.)
##########
File path: libminifi/src/core/yaml/YamlConfiguration.cpp
##########
@@ -49,35 +49,45 @@ YamlConfiguration::YamlConfiguration(const
std::shared_ptr<core::Repository>& re
logger_(logging::LoggerFactory<YamlConfiguration>::getLogger()) {}
std::unique_ptr<core::ProcessGroup>
YamlConfiguration::parseRootProcessGroupYaml(YAML::Node rootFlowNode) {
- utils::Identifier uuid;
- int version = 0;
-
- yaml::checkRequiredField(&rootFlowNode, "name", logger_,
- CONFIG_YAML_REMOTE_PROCESS_GROUP_KEY);
- std::string flowName = rootFlowNode["name"].as<std::string>();
-
- auto class_loader_functions = rootFlowNode["Class Loader Functions"];
+ auto flowControllerNode = rootFlowNode[CONFIG_YAML_FLOW_CONTROLLER_KEY];
+ auto class_loader_functions = flowControllerNode["Class Loader Functions"];
if (class_loader_functions && class_loader_functions.IsSequence()) {
for (auto function : class_loader_functions) {
registerResource(function.as<std::string>());
}
}
- std::string id = getOrGenerateId(&rootFlowNode);
+ auto rootGroup = parseProcessGroupYaml(flowControllerNode, rootFlowNode,
true);
+ this->name_ = rootGroup->getName();
+ return rootGroup;
+}
+
+std::unique_ptr<core::ProcessGroup>
YamlConfiguration::createProcessGroup(YAML::Node yamlNode, bool is_root) {
Review comment:
Some of these functions take a `YAML::Node` by value, some take a
`YAML::Node*`. I think they should all take a `const YAML::Node&` -- or at
least those functions which were added in this PR.
##########
File path: libminifi/src/core/FlowConfiguration.cpp
##########
@@ -116,6 +116,10 @@ std::unique_ptr<core::ProcessGroup>
FlowConfiguration::createRootProcessGroup(st
return std::unique_ptr<core::ProcessGroup>(new
core::ProcessGroup(core::ROOT_PROCESS_GROUP, name, uuid, version));
}
+std::unique_ptr<core::ProcessGroup>
FlowConfiguration::createSimpleProcessGroup(std::string name, utils::Identifier
& uuid, int version) {
+ return std::unique_ptr<core::ProcessGroup>(new
core::ProcessGroup(core::SIMPLE_PROCESS_GROUP, name, uuid, version));
Review comment:
this could be `utils::make_unique`, to avoid using `new` and to make it
slightly shorter
##########
File path: libminifi/src/core/ProcessGroup.cpp
##########
@@ -87,17 +87,17 @@ ProcessGroup::~ProcessGroup() {
for (auto&& connection : connections_) {
connection->drain(false);
}
-
- for (ProcessGroup* childGroup : child_process_groups_) {
- delete childGroup;
- }
}
bool ProcessGroup::isRootProcessGroup() {
- std::lock_guard<std::recursive_mutex> lock(mutex_);
return (type_ == ROOT_PROCESS_GROUP);
}
+bool ProcessGroup::isRemoteProcessGroup() {
+ return (type_ == REMOTE_PROCESS_GROUP);
+}
Review comment:
if we made `type_` const, it would be clearer why we don't need the lock
--
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
