fgerlits commented on code in PR #2065: URL: https://github.com/apache/nifi-minifi-cpp/pull/2065#discussion_r2889760609
########## libminifi/include/core/controller/ControllerService.h: ########## @@ -0,0 +1,158 @@ +/** + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#pragma once + +#include <memory> +#include <string> +#include <utility> +#include <vector> + +#include "minifi-cpp/properties/Configure.h" +#include "core/Core.h" +#include "core/ConfigurableComponentImpl.h" +#include "core/Connectable.h" +#include "minifi-cpp/core/controller/ControllerServiceApi.h" +#include "minifi-cpp/core/ControllerServiceApiDefinition.h" + +namespace org::apache::nifi::minifi::core::controller { + +enum ControllerServiceState { + DISABLED, + DISABLING, + ENABLING, + ENABLED +}; + +// Wrapper class to manage common functionalities of controller service implementations (properties, name, id, etc) +class ControllerService : public ConfigurableComponentImpl, public CoreComponentImpl { + class ControllerServiceDescriptorImpl : public ControllerServiceDescriptor { + public: + explicit ControllerServiceDescriptorImpl(ControllerService& impl): impl_(impl) {} + void setSupportedProperties(std::span<const PropertyReference> properties) override; + + private: + ControllerService& impl_; + }; + + class ControllerServiceContextImpl : public ControllerServiceContext { + public: + explicit ControllerServiceContextImpl(ControllerService& impl): impl_(impl) {} + [[nodiscard]] nonstd::expected<std::string, std::error_code> getProperty(std::string_view name) const override; + [[nodiscard]] nonstd::expected<std::vector<std::string>, std::error_code> getAllPropertyValues(std::string_view name) const override; + + private: + ControllerService& impl_; + }; + + public: + explicit ControllerService(std::string_view name, const utils::Identifier& uuid, std::unique_ptr<ControllerServiceApi> impl) + : CoreComponentImpl(name, uuid), + impl_(std::move(impl)), + configuration_(Configure::create()) { + current_state_ = DISABLED; + } + + void initialize() final { + ControllerServiceDescriptorImpl descriptor{*this}; + impl_->initialize(descriptor); + } + + bool supportsDynamicRelationships() const final { + return false; + } + + ~ControllerService() override { + notifyStop(); + } + + /** + * Replaces the configuration object within the controller service. + */ + void setConfiguration(const std::shared_ptr<Configure> &configuration) { + configuration_ = configuration; + } + + ControllerServiceState getState() const { + return current_state_.load(); + } + + /** + * Function is called when Controller Services are enabled and being run + */ + void onEnable() { + ControllerServiceContextImpl context{*this}; + std::vector<std::shared_ptr<ControllerServiceInterface>> service_interfaces; + for (auto& service : linked_services_) { + service_interfaces.emplace_back(std::shared_ptr<ControllerServiceInterface>(service, service->impl_->getControllerServiceInterface())); + } + impl_->onEnable(context, configuration_, service_interfaces); + } + + /** + * Function is called when Controller Services are disabled + */ + void notifyStop() { + impl_->notifyStop(); + } + + void setState(ControllerServiceState state) { + current_state_ = state; + if (state == DISABLED) { + notifyStop(); + } + } + + void setLinkedControllerServices(const std::vector<std::shared_ptr<controller::ControllerService>> &services) { + linked_services_ = services; + } + + template<typename T = ControllerServiceInterface> + gsl::not_null<T*> getImplementation() { + return gsl::make_not_null(dynamic_cast<T*>(impl_->getControllerServiceInterface())); + } + + bool supportsDynamicProperties() const final { + return false; + } + + static constexpr auto ImplementsApis = std::array<ControllerServiceApiDefinition, 0>{}; Review Comment: This is quite confusing: `core::controller::ControllerServiceApi` is the general API class, but we also have `core::ControllerServiceApi` which is a special thing needed to build the manifest, along with `ControllerServiceApiDefinition`, `ProvidesApi` and `ImplementsApis`. So the word "Api" is used in two completely different senses in lines 133 and 136. We should rename the old classes, or move them to a more specific namespace having to do with manifests, or both. ########## libminifi/include/core/controller/ControllerService.h: ########## @@ -0,0 +1,158 @@ +/** + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#pragma once + +#include <memory> +#include <string> +#include <utility> +#include <vector> + +#include "minifi-cpp/properties/Configure.h" +#include "core/Core.h" +#include "core/ConfigurableComponentImpl.h" +#include "core/Connectable.h" +#include "minifi-cpp/core/controller/ControllerServiceApi.h" +#include "minifi-cpp/core/ControllerServiceApiDefinition.h" + +namespace org::apache::nifi::minifi::core::controller { + +enum ControllerServiceState { + DISABLED, + DISABLING, + ENABLING, + ENABLED +}; + +// Wrapper class to manage common functionalities of controller service implementations (properties, name, id, etc) +class ControllerService : public ConfigurableComponentImpl, public CoreComponentImpl { + class ControllerServiceDescriptorImpl : public ControllerServiceDescriptor { + public: + explicit ControllerServiceDescriptorImpl(ControllerService& impl): impl_(impl) {} + void setSupportedProperties(std::span<const PropertyReference> properties) override; + + private: + ControllerService& impl_; + }; + + class ControllerServiceContextImpl : public ControllerServiceContext { + public: + explicit ControllerServiceContextImpl(ControllerService& impl): impl_(impl) {} + [[nodiscard]] nonstd::expected<std::string, std::error_code> getProperty(std::string_view name) const override; + [[nodiscard]] nonstd::expected<std::vector<std::string>, std::error_code> getAllPropertyValues(std::string_view name) const override; + + private: + ControllerService& impl_; + }; + + public: + explicit ControllerService(std::string_view name, const utils::Identifier& uuid, std::unique_ptr<ControllerServiceApi> impl) + : CoreComponentImpl(name, uuid), + impl_(std::move(impl)), + configuration_(Configure::create()) { + current_state_ = DISABLED; + } + + void initialize() final { + ControllerServiceDescriptorImpl descriptor{*this}; + impl_->initialize(descriptor); + } + + bool supportsDynamicRelationships() const final { + return false; + } + + ~ControllerService() override { + notifyStop(); + } + + /** + * Replaces the configuration object within the controller service. + */ + void setConfiguration(const std::shared_ptr<Configure> &configuration) { + configuration_ = configuration; + } + + ControllerServiceState getState() const { + return current_state_.load(); + } + + /** + * Function is called when Controller Services are enabled and being run + */ + void onEnable() { + ControllerServiceContextImpl context{*this}; + std::vector<std::shared_ptr<ControllerServiceInterface>> service_interfaces; + for (auto& service : linked_services_) { + service_interfaces.emplace_back(std::shared_ptr<ControllerServiceInterface>(service, service->impl_->getControllerServiceInterface())); + } + impl_->onEnable(context, configuration_, service_interfaces); + } + + /** + * Function is called when Controller Services are disabled + */ + void notifyStop() { + impl_->notifyStop(); + } + + void setState(ControllerServiceState state) { + current_state_ = state; + if (state == DISABLED) { + notifyStop(); + } + } + + void setLinkedControllerServices(const std::vector<std::shared_ptr<controller::ControllerService>> &services) { + linked_services_ = services; + } + + template<typename T = ControllerServiceInterface> + gsl::not_null<T*> getImplementation() { + return gsl::make_not_null(dynamic_cast<T*>(impl_->getControllerServiceInterface())); + } + + bool supportsDynamicProperties() const final { + return false; + } Review Comment: This used to return the value of the static `SupportsDynamicServices`, now it is a constant `false`. Why did this change? If we want to remove this option (I don't think we have any controller services where `SupportsDynamicProperties == true`), should we remove the static field, as well? ########## libminifi/src/controllers/NetworkPrioritizerService.cpp: ########## @@ -177,13 +166,6 @@ void NetworkPrioritizerService::onEnable() { logger_->log_trace("{} added to list of applied interfaces", ifc); } } - if (const auto is_default = getProperty(DefaultPrioritizer.name) | utils::andThen(parsing::parseBool)) { - if (*is_default) { - if (io::NetworkPrioritizerFactory::getInstance()->setPrioritizer(sharedFromThis<NetworkPrioritizerService>()) < 0) { - throw std::runtime_error("Can only have one prioritizer"); - } - } - } Review Comment: I'm not sure what this did before, but if we are not using the `DefaultPrioritizer` property any longer, then it should be either removed or deprecated and the doc string updated. ########## libminifi/test/libtest/unit/ControllerServiceUtils.h: ########## @@ -0,0 +1,40 @@ +/** +* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#pragma once + +#include <memory> +#include <string_view> + +#include "core/controller/ControllerService.h" + +namespace org::apache::nifi::minifi::test::utils { + +template<typename T> +std::unique_ptr<core::controller::ControllerService> make_controller_service(std::string_view name, std::optional<minifi::utils::Identifier> uuid = std::nullopt) { + if (!uuid) { + uuid = minifi::utils::IdGenerator::getIdGenerator()->generate(); + } + auto processor_impl = std::make_unique<T>(core::controller::ControllerServiceMetadata{ + .uuid = uuid.value(), + .name = std::string{name}, + .logger = minifi::core::logging::LoggerFactory<T>::getLogger(uuid.value()) + }); + return std::make_unique<core::controller::ControllerService>(name, uuid.value(), std::move(processor_impl)); Review Comment: typo: ```suggestion auto controller_service_impl = std::make_unique<T>(core::controller::ControllerServiceMetadata{ .uuid = uuid.value(), .name = std::string{name}, .logger = minifi::core::logging::LoggerFactory<T>::getLogger(uuid.value()) }); return std::make_unique<core::controller::ControllerService>(name, uuid.value(), std::move(controller_service_impl)); ``` ########## libminifi/test/unit/YamlFlowSerializerTests.cpp: ########## @@ -327,11 +327,11 @@ TEST_CASE("The encrypted flow configuration can be decrypted with the correct ke const auto controller_service_id = "b9801278-7b5d-4314-aed6-713fd4b5f933"; const auto* const controller_service_node_before = process_group_before->findControllerService(controller_service_id); REQUIRE(controller_service_node_before); - const auto* const controller_service_before = controller_service_node_before->getControllerServiceImplementation(); + const auto controller_service_before = controller_service_node_before->getControllerServiceImplementation(); REQUIRE(controller_service_node_before); const auto* const controller_service_node_after = process_group_after->findControllerService(controller_service_id); REQUIRE(controller_service_node_after); - const auto* const controller_service_after = controller_service_node_before->getControllerServiceImplementation(); + const auto controller_service_after = controller_service_node_after->getControllerServiceImplementation(); Review Comment: :+1: (I think this was my bug) -- 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]
