MINIFICPP-2669 - Reduce controller service api #2065
+1,034
−1,070
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adamdebreceni
force-pushed
the
MINIFICPP-2669
branch
from
36ff46b to
b61e730
Compare
adamdebreceni
force-pushed
the
MINIFICPP-2669
branch
from
fd945e9 to
6b53a92
Compare
szaszm
reviewed
Jan 21, 2026
| .artifact = "minifi-system", | ||
| .group = "org.apache.nifi.minifi", | ||
| .type = "org.apache.nifi.minifi.core.RecordSetReader", | ||
| .version = "1.0.0" |
Member
There was a problem hiding this comment.
I think the version of components shipped with minifi should always match the minifi version, what do you think?
| * 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. |
Member
There was a problem hiding this comment.
I don't understand this sentence, could you elaborate or rephrase?
Comment on lines
+110
to
+111
| ControllerServiceDescriptor* descriptor_{nullptr}; | ||
| ControllerServiceContext* context_{nullptr}; |
Member
There was a problem hiding this comment.
A comment describing the lifetimes (what guarantees that these will remain alive throughout the ControllerService lifetime) would be nice here.
| } | ||
| if (thread_manager_->isAboveMax(current_workers_)) { | ||
| auto max = thread_manager_->getMaxConcurrentTasks(); | ||
| auto max = 1; // thread_manager_->getMaxConcurrentTasks(); |
Member
There was a problem hiding this comment.
why did this change? leftover debug thing?
adamdebreceni
force-pushed
the
MINIFICPP-2669
branch
from
a1401a3 to
8f3c38b
Compare
There was a problem hiding this comment.
Pull request overview
Refactors MiNiFi C++ controller service APIs to reduce/reshape the exposed surface area, introducing a new “API/implementation” split and updating call sites across core, extensions, and tests.
Changes:
- Introduces new controller-service API types (API, context, descriptor, factory, metadata) and reworks controller service registration/lookup.
- Replaces many direct
ControllerServiceImplusages withControllerServiceBase+ControllerServiceInterfaceand wraps implementations in a newcore::controller::ControllerService. - Updates thread pool/controller service wiring and adjusts numerous unit/integration tests to the new controller service access patterns.
Reviewed changes
Copilot reviewed 118 out of 118 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| minifi-api/include/minifi-cpp/core/controller/ControllerServiceProvider.h | Removed legacy API header for controller service provider. |
| minifi-api/include/minifi-cpp/core/controller/ControllerServiceMetadata.h | Replaced old node/service API types with a minimal metadata struct. |
| minifi-api/include/minifi-cpp/core/controller/ControllerServiceInterface.h | Reduced controller service interface to a marker-style interface. |
| minifi-api/include/minifi-cpp/core/controller/ControllerServiceFactory.h | Added factory interface for creating controller service API implementations. |
| minifi-api/include/minifi-cpp/core/controller/ControllerServiceDescriptor.h | Added descriptor API for declaring supported properties. |
| minifi-api/include/minifi-cpp/core/controller/ControllerServiceContext.h | Added context API for property access during enable. |
| minifi-api/include/minifi-cpp/core/controller/ControllerServiceApi.h | Added new controller-service API surface (initialize/onEnable/notifyStop). |
| minifi-api/include/minifi-cpp/core/ProcessContext.h | Switched controller service return type to ControllerServiceInterface. |
| minifi-api/include/minifi-cpp/core/ControllerServiceApiDefinition.h | Added version field to controller service API definition. |
| minifi-api/include/minifi-cpp/core/ClassLoader.h | Added overload to register controller service factories. |
| minifi-api/include/minifi-cpp/controllers/ThreadManagementService.h | Migrated to ControllerServiceInterface. |
| minifi-api/include/minifi-cpp/controllers/SSLContextServiceInterface.h | Migrated to ControllerServiceInterface and added API version. |
| minifi-api/include/minifi-cpp/controllers/RecordSetWriter.h | Migrated to ControllerServiceInterface and added API version. |
| minifi-api/include/minifi-cpp/controllers/RecordSetReader.h | Migrated to ControllerServiceInterface and added API version. |
| minifi-api/include/minifi-cpp/controllers/AttributeProviderService.h | Migrated to ControllerServiceInterface. |
| libminifi/test/unit/YamlFlowSerializerTests.cpp | Updated controller service accessors to new return types. |
| libminifi/test/unit/UpdatePolicyTests.cpp | Updated controller service creation and access to implementation. |
| libminifi/test/unit/ProcessorConfigUtilsTests.cpp | Updated controller-service parsing tests for new wrappers/interfaces. |
| libminifi/test/unit/NetworkPrioritizerServiceTests.cpp | Updated tests to use wrapped controller services and implementation accessors. |
| libminifi/test/unit/NetUtilsTest.cpp | Updated SSL context service retrieval and usage. |
| libminifi/test/unit/JsonFlowSerializerTests.cpp | Updated controller service accessors to new return types. |
| libminifi/test/unit/ComponentManifestTests.cpp | Updated example controller service type hierarchy for new APIs. |
| libminifi/test/libtest/unit/MockClasses.h | Updated mock controller service usage for new ControllerServiceInterface. |
| libminifi/test/libtest/unit/ControllerServiceUtils.h | Added test helper for constructing wrapped controller services. |
| libminifi/test/keyvalue-tests/VolatileMapStateStorageTest.cpp | Updated controller service implementation retrieval helper usage. |
| libminifi/test/keyvalue-tests/PersistentStateStorageTest.cpp | Updated controller service implementation retrieval helper usage. |
| libminifi/test/integration/ControllerServiceIntegrationTests.cpp | Updated controller service implementation retrieval helper usage. |
| libminifi/test/integration/C2ControllerEnableFailureTest.cpp | Updated dummy controller service to new base/interface split. |
| libminifi/src/core/state/nodes/ResponseNodeLoader.cpp | Updated UpdatePolicy controller retrieval to new implementation access path. |
| libminifi/src/core/logging/LoggerConfiguration.cpp | Switched SSLContextService creation to createAndEnable. |
| libminifi/src/core/controller/ControllerServiceProvider.cpp | Renamed provider impl usage to new provider type. |
| libminifi/src/core/controller/ControllerServiceNode.cpp | Updated node implementation accessors and signatures. |
| libminifi/src/core/ClassLoader.cpp | Added wrapper to adapt controller service factories into object factories. |
| libminifi/src/controllers/UpdatePolicyControllerService.cpp | Removed now-unneeded legacy lifecycle methods. |
| libminifi/src/controllers/SSLContextService.cpp | Added createAndEnable helper using new wrapper service type. |
| libminifi/src/controllers/NetworkPrioritizerService.cpp | Updated prioritizer token handling and linkage with new API model. |
| libminifi/src/c2/protocols/RESTSender.cpp | Switched SSLContextService creation to createAndEnable. |
| libminifi/src/c2/ControllerSocketProtocol.cpp | Switched SSLContextService creation to createAndEnable. |
| libminifi/src/c2/C2Agent.cpp | Updated UpdatePolicy controller retrieval to new implementation access path. |
| libminifi/src/RemoteProcessGroupPort.cpp | Updated controller service lookup return type and SSL fallback creation. |
| libminifi/src/FlowController.cpp | Updated thread pool controller service provider wiring. |
| libminifi/include/io/NetworkPrioritizer.h | Removed global prioritizer factory and API method from interface. |
| libminifi/include/core/controller/StandardControllerServiceProvider.h | Updated to new provider base type and base include. |
| libminifi/include/core/controller/StandardControllerServiceNode.h | Updated to new node base type and includes. |
| libminifi/include/core/controller/ForwardingControllerServiceProvider.h | Updated to new provider base and removed deprecated methods. |
| libminifi/include/core/controller/ControllerServiceProvider.h | Consolidated provider API/impl and removed linked-service state helpers. |
| libminifi/include/core/controller/ControllerServiceNodeMap.h | Updated include paths for new controller service node header. |
| libminifi/include/core/controller/ControllerServiceNode.h | Replaced old node interface with new concrete base and templated impl accessor. |
| libminifi/include/core/controller/ControllerServiceLookup.h | Removed enabled/enabling/name helpers; kept lookup methods only. |
| libminifi/include/core/controller/ControllerService.h | Added new wrapper controller service type around ControllerServiceApi. |
| libminifi/include/core/ProcessGroup.h | Updated include path for controller service node. |
| libminifi/include/core/ProcessContextImpl.h | Updated to return ControllerServiceInterface and use templated node accessor. |
| libminifi/include/core/FlowConfiguration.h | Updated include path for controller service node. |
| libminifi/include/controllers/UpdatePolicyControllerService.h | Migrated to ControllerServiceBase + ControllerServiceInterface. |
| libminifi/include/controllers/ThreadManagementService.h | Migrated to ControllerServiceBase and updated initialization. |
| libminifi/include/controllers/SSLContextService.h | Migrated to ControllerServiceBase and added createAndEnable. |
| libminifi/include/controllers/NetworkPrioritizerService.h | Migrated to ControllerServiceBase + ControllerServiceInterface and embedded prioritizer state. |
| libminifi/include/c2/HeartbeatReporter.h | Updated include path for controller service provider. |
| libminifi/include/c2/ControllerSocketProtocol.h | Updated include path for controller service provider. |
| libminifi/include/c2/C2Protocol.h | Updated include path for controller service provider. |
| libminifi/include/SchedulingAgent.h | Updated include path for controller service provider. |
| libminifi/include/FlowController.h | Updated include path for controller service provider. |
| extensions/standard-processors/tests/unit/XMLRecordSetWriterTests.cpp | Updated tests to use wrapped controller services and implementation accessors. |
| extensions/standard-processors/tests/unit/XMLReaderTests.cpp | Updated tests to use wrapped controller services and implementation accessors. |
| extensions/standard-processors/tests/unit/TailFileTests.cpp | Updated test controller service mock to match new interface expectations. |
| extensions/standard-processors/tests/unit/JsonRecordTests.cpp | Updated tests to use wrapped controller services and implementation accessors. |
| extensions/standard-processors/tests/unit/ControllerServiceTests.cpp | Updated tests to construct new wrapped controller service type. |
| extensions/standard-processors/controllers/XMLRecordSetWriter.h | Updated service construction style and removed legacy common macros. |
| extensions/standard-processors/controllers/XMLReader.h | Updated service construction style and removed legacy common macros. |
| extensions/standard-processors/controllers/VolatileMapStateStorage.h | Updated construction style and removed legacy common macros. |
| extensions/standard-processors/controllers/VolatileMapStateStorage.cpp | Updated base initialization call to ControllerServiceBase. |
| extensions/standard-processors/controllers/PersistentMapStateStorage.h | Updated construction style and removed legacy common macros. |
| extensions/standard-processors/controllers/PersistentMapStateStorage.cpp | Updated base initialization call to ControllerServiceBase. |
| extensions/standard-processors/controllers/JsonTreeReader.h | Updated construction style and removed legacy common macros. |
| extensions/standard-processors/controllers/JsonRecordSetWriter.h | Updated construction style and removed legacy common macros. |
| extensions/sql/tests/mocks/MockODBCService.h | Updated construction style and removed legacy common macros. |
| extensions/sql/services/ODBCConnector.h | Updated ODBCService construction style and removed legacy common macros. |
| extensions/sql/services/DatabaseService.h | Migrated to ControllerServiceBase + ControllerServiceInterface. |
| extensions/sql/services/DatabaseService.cpp | Updated base initialization call to ControllerServiceBase. |
| extensions/smb/tests/utils/MockSmbConnectionControllerService.h | Updated test mock to remove legacy controller-service macros. |
| extensions/smb/tests/SmbConnectionControllerServiceTests.cpp | Updated to use templated node implementation accessor. |
| extensions/smb/tests/PutSmbTests.cpp | Updated to use templated node implementation accessor. |
| extensions/smb/tests/ListSmbTests.cpp | Updated to use templated node implementation accessor. |
| extensions/smb/tests/ListAndFetchSmbTests.cpp | Updated to use templated node implementation accessor. |
| extensions/smb/tests/FetchSmbTests.cpp | Updated to use templated node implementation accessor. |
| extensions/smb/SmbConnectionControllerService.h | Migrated to ControllerServiceBase + ControllerServiceInterface. |
| extensions/rocksdb-repos/controllers/RocksDbStateStorage.h | Updated construction style and removed legacy common macros. |
| extensions/rocksdb-repos/controllers/RocksDbStateStorage.cpp | Updated base initialization call to ControllerServiceBase. |
| extensions/kubernetes/controllerservice/KubernetesControllerService.h | Updated construction style and removed legacy common macros. |
| extensions/kubernetes/controllerservice/KubernetesControllerService.cpp | Updated base initialization call to ControllerServiceBase. |
| extensions/gcp/tests/GCPCredentialsControllerServiceTests.cpp | Updated to use templated node implementation accessor. |
| extensions/gcp/controllerservices/GCPCredentialsControllerService.h | Migrated to ControllerServiceBase + ControllerServiceInterface. |
| extensions/elasticsearch/ElasticsearchCredentialsControllerService.h | Migrated to ControllerServiceBase + ControllerServiceInterface. |
| extensions/couchbase/tests/PutCouchbaseKeyTests.cpp | Updated to use templated node implementation accessor. |
| extensions/couchbase/tests/MockCouchbaseClusterService.h | Updated test mock to remove legacy controller-service macros. |
| extensions/couchbase/tests/GetCouchbaseKeyTests.cpp | Updated to use templated node implementation accessor. |
| extensions/couchbase/controllerservices/CouchbaseClusterService.h | Migrated to ControllerServiceBase + ControllerServiceInterface. |
| extensions/civetweb/tests/ListenHTTPTests.cpp | Switched SSLContextService creation to createAndEnable. |
| extensions/azure/processors/AzureStorageProcessorBase.cpp | Updated controller service lookup return type. |
| extensions/azure/controllerservices/AzureStorageCredentialsService.h | Migrated to ControllerServiceBase + ControllerServiceInterface. |
| extensions/aws/tests/MultipartUploadStateStorageTest.cpp | Updated state storage construction to use metadata-based constructor. |
| extensions/aws/tests/AWSCredentialsServiceTest.cpp | Updated to use templated node implementation accessor. |
| extensions/aws/controllerservices/AWSCredentialsService.h | Migrated to ControllerServiceBase + ControllerServiceInterface. |
| extension-framework/src/controllers/keyvalue/KeyValueStateStorage.cpp | Removed legacy constructor in favor of new base. |
| extension-framework/include/utils/ProcessorConfigUtils.h | Updated controller service parsing to use ControllerServiceInterface. |
| extension-framework/include/controllers/keyvalue/KeyValueStateStorage.h | Migrated to ControllerServiceBase + ControllerServiceInterface. |
| extension-framework/include/controllers/RecordSetWriter.h | Migrated to ControllerServiceBase + interface exposure. |
| extension-framework/include/controllers/RecordSetReader.h | Migrated to ControllerServiceBase + interface exposure. |
| extension-framework/include/controllers/AttributeProviderService.h | Migrated to ControllerServiceBase + interface exposure. |
| encrypt-config/FlowConfigEncryptor.cpp | Updated controller service implementation retrieval call site. |
| core-framework/src/utils/ThreadPool.cpp | Updated controller service provider API and thread manager lookup. |
| core-framework/include/utils/ThreadPool.h | Updated provider API from lookup pointer to function callback. |
| core-framework/include/core/controller/ControllerServiceFactoryImpl.h | Added templated factory implementation for controller services. |
| core-framework/include/core/controller/ControllerServiceBase.h | Added new base implementation for controller service APIs. |
| core-framework/include/core/controller/ControllerService.h | Removed legacy ControllerServiceImpl header. |
| core-framework/include/core/Resource.h | Updated resource registration to also register controller services via factories. |
| controller/tests/ControllerTests.cpp | Switched SSLContextService creation to createAndEnable. |
| controller/MiNiFiController.cpp | Switched SSLContextService creation to createAndEnable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+19
to
+30
| #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; | ||
| }; |
There was a problem hiding this 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 , , and <system_error>).
Comment on lines
+19
to
+33
| #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; |
There was a problem hiding this comment.
ControllerServiceApi.h uses std::shared_ptr and std::vector in its public virtual methods but does not include / . 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.
| } | ||
| if (thread_manager_->isAboveMax(current_workers_)) { | ||
| auto max = thread_manager_->getMaxConcurrentTasks(); | ||
| auto max = 1; // thread_manager_->getMaxConcurrentTasks(); |
There was a problem hiding this 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.
Suggested change
| auto max = 1; // thread_manager_->getMaxConcurrentTasks(); | |
| auto max = thread_manager_->getMaxConcurrentTasks(); |
Comment on lines
+129
to
+132
| template<typename T = ControllerServiceInterface> | ||
| gsl::not_null<T*> getImplementation() { | ||
| return gsl::make_not_null(dynamic_cast<T*>(impl_.get())); | ||
| } |
There was a problem hiding this 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).
| 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(); |
There was a problem hiding this 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.
| 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(); |
There was a problem hiding this 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit?
For code changes:
For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.