Copilot commented on code in PR #2065: URL: https://github.com/apache/nifi-minifi-cpp/pull/2065#discussion_r2754178578
########## minifi-api/include/minifi-cpp/core/controller/ControllerServiceContext.h: ########## @@ -0,0 +1,32 @@ +/** +* 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 <string_view> +#include "utils/expected.h" + +namespace org::apache::nifi::minifi::core::controller { + +class ControllerServiceContext { + public: + virtual ~ControllerServiceContext() = default; + + [[nodiscard]] virtual nonstd::expected<std::string, std::error_code> getProperty(std::string_view name) const = 0; + [[nodiscard]] virtual nonstd::expected<std::vector<std::string>, std::error_code> getAllPropertyValues(std::string_view name) const = 0; +}; Review Comment: This header uses std::string, std::vector and std::error_code in the method return types, but it only includes <string_view> and utils/expected.h. This makes the header non-self-contained and can fail to compile depending on include order. Add the required standard headers (at least <string>, <vector>, and <system_error>). ########## minifi-api/include/minifi-cpp/core/controller/ControllerServiceApi.h: ########## @@ -0,0 +1,36 @@ +/** +* 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 "ControllerServiceInterface.h" +#include "ControllerServiceContext.h" +#include "ControllerServiceDescriptor.h" +#include "minifi-cpp/properties/Configure.h" + +namespace org::apache::nifi::minifi::core::controller { + +class ControllerServiceApi { + public: + virtual ~ControllerServiceApi() = default; + + virtual void initialize(ControllerServiceDescriptor& descriptor) = 0; + virtual void onEnable(ControllerServiceContext& context, const std::shared_ptr<Configure>& configuration, const std::vector<std::shared_ptr<ControllerServiceInterface>>& linked_services) = 0; + virtual void notifyStop() = 0; + virtual ControllerServiceInterface* getControllerServiceInterface() = 0; Review Comment: ControllerServiceApi.h uses std::shared_ptr and std::vector in its public virtual methods but does not include <memory> / <vector>. Relying on transitive includes makes this header fragile for API consumers; add the missing standard includes (or forward declarations where feasible) so it compiles standalone. ########## libminifi/include/core/controller/ControllerService.h: ########## @@ -0,0 +1,163 @@ +/** + * + * 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 +}; + +/** + * Controller Service base class that contains some pure virtual methods. + * + * Design: OnEnable is executed when the controller service is being enabled. + * Note that keeping state here must be protected in this function. + */ +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_.get())); + } Review Comment: ControllerService::getImplementation() ignores ControllerServiceApi::getControllerServiceInterface() and instead dynamic_casts impl_.get() to T*. This can break as soon as an implementation returns an interface pointer that is not the same object as the API implementation, and it will also crash if the cast fails (gsl::make_not_null on nullptr). Consider basing this on impl_->getControllerServiceInterface() and returning a nullable pointer/expected (or at least enforce the cast with a clear error). ########## libminifi/test/unit/JsonFlowSerializerTests.cpp: ########## @@ -712,11 +712,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_before->getControllerServiceImplementation(); Review Comment: This retrieves the "after" controller service implementation from controller_service_node_before instead of controller_service_node_after, so the test is likely comparing the same service instance twice. Fetch the implementation from controller_service_node_after here. ########## core-framework/src/utils/ThreadPool.cpp: ########## @@ -152,7 +152,7 @@ void ThreadPool::manageWorkers() { continue; } if (thread_manager_->isAboveMax(current_workers_)) { - auto max = thread_manager_->getMaxConcurrentTasks(); + auto max = 1; // thread_manager_->getMaxConcurrentTasks(); Review Comment: This hard-codes the maximum worker count to 1 (with the previous getMaxConcurrentTasks() call commented out). That changes thread pool scaling behavior and will likely force unnecessary thread reductions under load. Restore using thread_manager_->getMaxConcurrentTasks() (or an equivalent) and remove the temporary constant. ```suggestion auto max = thread_manager_->getMaxConcurrentTasks(); ``` ########## 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_before->getControllerServiceImplementation(); Review Comment: This retrieves the "after" controller service implementation from controller_service_node_before instead of controller_service_node_after, so the test is likely comparing the same service instance twice. Fetch the implementation from controller_service_node_after here. -- 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]
