fgerlits commented on code in PR #2098:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2098#discussion_r2960258116
##########
libminifi/src/core/FlowConfiguration.cpp:
##########
@@ -195,8 +195,8 @@ std::unique_ptr<minifi::Connection>
FlowConfiguration::createConnection(const st
}
std::shared_ptr<core::controller::ControllerServiceNode>
FlowConfiguration::createControllerService(const std::string &class_name, const
std::string &name,
- const utils::Identifier& uuid) {
- std::shared_ptr<core::controller::ControllerServiceNode>
controllerServicesNode = service_provider_->createControllerService(class_name,
name);
+ const utils::Identifier& uuid, ProcessGroup* parent) {
+ std::shared_ptr<core::controller::ControllerServiceNode>
controllerServicesNode = service_provider_->createControllerService(class_name,
name, parent, uuid.to_string());
Review Comment:
also nitpicking, but I would make the `uuid` the primary key and `name` the
alternative key:
```suggestion
std::shared_ptr<core::controller::ControllerServiceNode>
controllerServicesNode = service_provider_->createControllerService(class_name,
uuid.to_string(), parent, name);
```
##########
libminifi/include/core/controller/ControllerServiceNodeMap.h:
##########
@@ -43,18 +43,25 @@ class ControllerServiceNodeMap {
ControllerServiceNode* get(const std::string &id) const;
ControllerServiceNode* get(const std::string &id, const utils::Identifier
&processor_or_controller_uuid) const;
- bool put(const std::string &id, const std::shared_ptr<ControllerServiceNode>
&serviceNode);
- bool put(const std::string &id, ProcessGroup* process_group);
+ bool put(std::string id, std::shared_ptr<ControllerServiceNode>
controller_service_node, ProcessGroup* parent_group);
+
+ bool registerAlternativeKey(std::string primary_key, std::string
alternative_key);
void clear();
std::vector<std::shared_ptr<ControllerServiceNode>>
getAllControllerServices() const;
protected:
mutable std::mutex mutex_;
- // Map of controller service id to the controller service node
- std::map<std::string, std::shared_ptr<ControllerServiceNode>>
controller_service_nodes_;
- // Map of controller service id to the process group that contains it
- std::map<std::string, gsl::not_null<ProcessGroup*>> process_groups_;
+
+ struct ServiceEntry {
+ std::shared_ptr<ControllerServiceNode> controller_service_node;
+ ProcessGroup* parent_group;
+ };
+
+ const ServiceEntry* getEntry(std::string_view primary_key, const
std::scoped_lock<std::mutex>& mutex) const;
+
+ std::map<std::string, ServiceEntry, std::less<>> services_;
+ std::map<std::string, std::string, std::less<>> alternative_keys;
Review Comment:
nitpicking, but this should be `alternative_keys_`
##########
libminifi/src/core/controller/ControllerServiceNodeMap.cpp:
##########
@@ -17,81 +17,93 @@
*/
#include "core/controller/ControllerServiceNodeMap.h"
+
+#include <ranges>
+
#include "core/ProcessGroup.h"
namespace org::apache::nifi::minifi::core::controller {
ControllerServiceNode* ControllerServiceNodeMap::get(const std::string &id)
const {
- std::lock_guard<std::mutex> lock(mutex_);
- auto exists = controller_service_nodes_.find(id);
- if (exists != controller_service_nodes_.end())
- return exists->second.get();
- else
- return nullptr;
+ const std::scoped_lock lock(mutex_);
+ if (const auto entry = getEntry(id, lock)) {
+ return entry->controller_service_node.get();
+ }
+ return nullptr;
}
ControllerServiceNode* ControllerServiceNodeMap::get(const std::string &id,
const utils::Identifier& processor_or_controller_uuid) const {
- std::lock_guard<std::mutex> lock(mutex_);
- ControllerServiceNode* controller = nullptr;
- auto exists = controller_service_nodes_.find(id);
- if (exists != controller_service_nodes_.end()) {
- controller = exists->second.get();
- } else {
+ const std::scoped_lock lock(mutex_);
+ const auto entry = getEntry(id, lock);
+ if (!entry || !entry->parent_group) {
return nullptr;
}
- auto process_group_of_controller_exists = process_groups_.find(id);
- ProcessGroup* process_group = nullptr;
- if (process_group_of_controller_exists != process_groups_.end()) {
- process_group = process_group_of_controller_exists->second;
- } else {
- return nullptr;
- }
- if (process_group->findProcessorById(processor_or_controller_uuid,
ProcessGroup::Traverse::IncludeChildren)) {
- return controller;
+ if (entry->parent_group->findProcessorById(processor_or_controller_uuid,
ProcessGroup::Traverse::IncludeChildren)) {
+ return entry->controller_service_node.get();
}
- if
(process_group->findControllerService(processor_or_controller_uuid.to_string(),
ProcessGroup::Traverse::IncludeChildren)) {
- return controller;
+ if
(entry->parent_group->findControllerService(processor_or_controller_uuid.to_string(),
ProcessGroup::Traverse::IncludeChildren)) {
+ return entry->controller_service_node.get();
}
return nullptr;
}
-bool ControllerServiceNodeMap::put(const std::string &id, const
std::shared_ptr<ControllerServiceNode> &serviceNode) {
- if (id.empty() || serviceNode == nullptr)
+bool ControllerServiceNodeMap::put(std::string id,
std::shared_ptr<ControllerServiceNode> controller_service_node,
+ ProcessGroup* parent_group) {
+ std::scoped_lock lock(mutex_);
+ if (id.empty() || controller_service_node == nullptr ||
alternative_keys.contains(id)) {
return false;
- std::lock_guard<std::mutex> lock(mutex_);
- controller_service_nodes_[id] = serviceNode;
- return true;
+ }
+ auto [_it, success] = services_.emplace(std::move(id),
ServiceEntry{.controller_service_node = std::move(controller_service_node),
.parent_group = parent_group});
+ return success;
}
-bool ControllerServiceNodeMap::put(const std::string &id, ProcessGroup*
process_group) {
- if (id.empty() || process_group == nullptr)
- return false;
- std::lock_guard<std::mutex> lock(mutex_);
- process_groups_.emplace(id, gsl::make_not_null(process_group));
- return true;
-}
void ControllerServiceNodeMap::clear() {
- std::lock_guard<std::mutex> lock(mutex_);
- for (const auto& [id, node] : controller_service_nodes_) {
- node->disable();
+ std::scoped_lock lock(mutex_);
+ for (const auto& node: services_ | std::views::values) {
+ node.controller_service_node->disable();
Review Comment:
can we call this `service_entry`, please?
```suggestion
for (const auto& service_entry: services_ | std::views::values) {
service_entry.controller_service_node->disable();
```
##########
libminifi/test/libtest/unit/TestBase.cpp:
##########
@@ -368,7 +368,7 @@
std::shared_ptr<minifi::core::controller::ControllerServiceNode> TestPlan::addCo
minifi::utils::Identifier uuid =
minifi::utils::IdGenerator::getIdGenerator()->generate();
std::shared_ptr<minifi::core::controller::ControllerServiceNode>
controller_service_node =
- controller_services_provider_->createControllerService(controller_name,
name);
+ controller_services_provider_->createControllerService(controller_name,
name, root_process_group_.get(), uuid.to_string());
Review Comment:
switch `name` and `uuid` here, too?
```suggestion
controller_services_provider_->createControllerService(controller_name,
uuid.to_string(), root_process_group_.get(), name);
```
--
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]
