szaszm commented on code in PR #1930:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1930#discussion_r2190545198
##########
minifi-api/include/minifi-cpp/core/ClassLoader.h:
##########
@@ -62,15 +65,19 @@ class ClassLoader {
*/
virtual void registerClass(const std::string &clazz,
std::unique_ptr<ObjectFactory> factory) = 0;
+ virtual void registerClass(const std::string &clazz,
std::unique_ptr<ProcessorFactory> factory) = 0;
+
virtual void unregisterClass(const std::string& clazz) = 0;
- virtual std::optional<std::string> getGroupForClass(const std::string
&class_name) const = 0;
+ [[nodiscard]] virtual std::optional<std::string> getGroupForClass(const
std::string &class_name) const = 0;
+
+ [[nodiscard]] virtual std::unique_ptr<CoreComponent> instantiate(const
std::string &class_name, const std::string &name,
std::function<bool(CoreComponent*)> filter) = 0;
- virtual std::unique_ptr<CoreComponent> instantiate(const std::string
&class_name, const std::string &name, std::function<bool(CoreComponent*)>
filter) = 0;
+ [[nodiscard]] virtual std::unique_ptr<CoreComponent> instantiate(const
std::string &class_name, const utils::Identifier &uuid,
std::function<bool(CoreComponent*)> filter) = 0;
- virtual std::unique_ptr<CoreComponent> instantiate(const std::string
&class_name, const utils::Identifier &uuid, std::function<bool(CoreComponent*)>
filter) = 0;
+ [[nodiscard]] virtual std::unique_ptr<CoreComponent> instantiate(const
std::string &class_name, const std::string &name, const utils::Identifier
&uuid, std::function<bool(CoreComponent*)> filter) = 0;
Review Comment:
do we need so many overloads? Since most of them just delegate to other
overloads, maybe it could be done with one virtual implementation and maybe a
few nonvirtual convenience wrappers
##########
core-framework/include/core/ProcessorFactory.h:
##########
@@ -0,0 +1,57 @@
+/**
+ * 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>
+#include <memory>
+#include <utility>
+#include "ClassName.h"
+#include "minifi-cpp/core/ProcessorFactory.h"
+
+namespace org::apache::nifi::minifi::core {
+
+template<class T>
+class ProcessorFactoryImpl : public ProcessorFactory {
+ public:
+ ProcessorFactoryImpl()
+ : class_name_(core::className<T>()) {
+ }
+
+ explicit ProcessorFactoryImpl(std::string group_name)
+ : group_name_(std::move(group_name)),
+ class_name_(core::className<T>()) {
+ }
+
+ std::string getGroupName() const override {
+ return group_name_;
+ }
+
+ std::unique_ptr<ProcessorApi> create(ProcessorMetadata metadata) override {
+ return std::make_unique<T>(metadata);
+ }
+
+ std::string getClassName() const override {
+ return class_name_;
+ }
+
+ protected:
+ std::string group_name_;
+ std::string class_name_;
Review Comment:
At least class_name_, but maybe both could become a string_view:
- class_name_ comes from core::className which returns a string_view to
static strings
- group_name_ seems to come from different places, but they seem to be
static strings too.
##########
libminifi/include/core/reporting/SiteToSiteProvenanceReportingTask.h:
##########
@@ -35,8 +35,8 @@ namespace org::apache::nifi::minifi::core::reporting {
class SiteToSiteProvenanceReportingTask : public
minifi::RemoteProcessorGroupPort {
public:
explicit SiteToSiteProvenanceReportingTask(std::shared_ptr<Configure>
configure)
- : minifi::RemoteProcessorGroupPort(ReportTaskName, "",
std::move(configure)),
-
logger_(logging::LoggerFactory<SiteToSiteProvenanceReportingTask>::getLogger())
{
+ : minifi::RemoteProcessorGroupPort(ReportTaskName, "",
std::move(configure),
+
utils::IdGenerator::getIdGenerator()->generate(),
logging::LoggerFactory<SiteToSiteProvenanceReportingTask>::getLogger()) {
Review Comment:
```suggestion
: minifi::RemoteProcessorGroupPort(ReportTaskName, "",
std::move(configure),
utils::IdGenerator::getIdGenerator()->generate(),
logging::LoggerFactory<SiteToSiteProvenanceReportingTask>::getLogger()) {
```
##########
libminifi/src/core/FlowConfiguration.cpp:
##########
@@ -71,28 +71,17 @@ FlowConfiguration::~FlowConfiguration() {
}
}
-std::unique_ptr<core::Processor> FlowConfiguration::createProcessor(const
std::string &name, const utils::Identifier &uuid) {
- auto processor = minifi::processors::ProcessorUtils::createProcessor(name,
name, uuid);
+std::unique_ptr<core::Processor> FlowConfiguration::createProcessor(const
std::string &class_short, const std::string &fullclass, const std::string
&name, const utils::Identifier &uuid) {
+ auto processor =
minifi::processors::ProcessorUtils::createProcessor(class_short, fullclass,
name, uuid);
if (nullptr == processor) {
- logger_->log_error("No Processor defined for {}", name);
- return nullptr;
- }
- return processor;
-}
-
-std::unique_ptr<core::Processor> FlowConfiguration::createProcessor(const
std::string &name, const std::string &fullname, const utils::Identifier &uuid) {
- auto processor = minifi::processors::ProcessorUtils::createProcessor(name,
fullname, uuid);
- if (nullptr == processor) {
- logger_->log_error("No Processor defined for {}", fullname);
+ logger_->log_error("No Processor defined for {}", fullclass);
return nullptr;
}
return processor;
}
std::unique_ptr<core::reporting::SiteToSiteProvenanceReportingTask>
FlowConfiguration::createProvenanceReportTask() {
- auto processor =
std::make_unique<org::apache::nifi::minifi::core::reporting::SiteToSiteProvenanceReportingTask>(this->configuration_);
- processor->initialize();
- return processor;
+ return
std::make_unique<org::apache::nifi::minifi::core::reporting::SiteToSiteProvenanceReportingTask>(this->configuration_);
Review Comment:
is this initialize call safe to remove?
##########
core-framework/include/core/ProcessorImpl.h:
##########
@@ -0,0 +1,160 @@
+/**
+ * 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 <algorithm>
+#include <atomic>
+#include <chrono>
+#include <condition_variable>
+#include <functional>
+#include <memory>
+#include <mutex>
+#include <string>
+#include <string_view>
+#include <unordered_set>
+#include <unordered_map>
+#include <utility>
+#include <vector>
Review Comment:
My IDE shows that half of these are unnecessary: algorithm, chrono,
string_view, unordered_set, unordered_map, utility, vector. I also see missing
includes, like array, span
##########
libminifi/test/unit/ProcessorConfigUtilsTests.cpp:
##########
@@ -46,8 +53,11 @@ TEST_CASE("Parse enum property") {
static constexpr auto prop =
PropertyDefinitionBuilder<magic_enum::enum_count<TestEnum>()>::createProperty("prop")
.withAllowedValues(magic_enum::enum_names<TestEnum>())
.build();
- auto proc = std::make_shared<TestProcessor>("test-proc");
- proc->setSupportedProperties(std::to_array<core::PropertyReference>({prop}));
+ auto proc = minifi::test::utils::make_processor<TestProcessor>("test-proc");
+ proc->getImpl<TestProcessor>().on_initialize_ = [&] (auto& self) {
+
self.setSupportedProperties(std::to_array<core::PropertyReference>({prop}));
+ };
+ proc->initialize();
Review Comment:
why was this changed?
##########
extensions/standard-processors/processors/DefragmentText.h:
##########
@@ -116,7 +113,6 @@ class DefragmentText : public core::ProcessorImpl {
void onSchedule(core::ProcessContext& context, core::ProcessSessionFactory&
session_factory) override;
void onTrigger(core::ProcessContext& context, core::ProcessSession& session)
override;
void restore(const std::shared_ptr<core::FlowFile>& flowFile) override;
- std::set<core::Connectable*> getOutGoingConnections(const std::string
&relationship) override;
Review Comment:
why is this gone?
##########
libminifi/test/libtest/unit/TestUtils.h:
##########
@@ -210,18 +213,17 @@ inline std::error_code hide_file(const
std::filesystem::path& file_name) {
#endif /* WIN32 */
template<typename T>
-concept NetworkingProcessor = std::derived_from<T, minifi::core::Processor>
- && requires(T x) {
- {T::Port} -> std::convertible_to<core::PropertyReference>;
- {x.getPort()} -> std::convertible_to<uint16_t>;
- }; // NOLINT(readability/braces)
+concept NetworkingProcessor = requires(T x) {
+ {T::Port} -> std::convertible_to<core::PropertyReference>;
+ {x.getPort()} -> std::convertible_to<uint16_t>;
+}; // NOLINT(readability/braces)
Review Comment:
Why is the subclass status no longer required? All processors need to
implement the ProcessorApi at the moment
##########
extensions/standard-processors/processors/RouteText.cpp:
##########
@@ -57,18 +54,10 @@ void RouteText::onSchedule(core::ProcessContext& context,
core::ProcessSessionFa
group_fallback_ = context.getProperty(GroupingFallbackValue).value_or("");
- {
- const auto static_relationships = RouteText::Relationships;
- std::vector<core::RelationshipDefinition>
relationships(static_relationships.begin(), static_relationships.end());
-
- for (const auto& property_name : getDynamicPropertyKeys()) {
- core::RelationshipDefinition rel{property_name, "Dynamic Route"};
- dynamic_relationships_[property_name] = rel;
- relationships.push_back(rel);
- logger_->log_info("RouteText registered dynamic route '{}'",
property_name);
- }
-
- setSupportedRelationships(relationships);
Review Comment:
this is no longer called a second time with the dynamic relationships added.
Is this on purpose? Why?
##########
libminifi/test/libtest/unit/TestBase.h:
##########
@@ -201,13 +201,56 @@ class TempDirectory {
bool is_owner_;
};
+template<typename T>
+class TypedProcessorWrapper {
+ public:
+ TypedProcessorWrapper() = default;
+ TypedProcessorWrapper(core::Processor* proc): proc_(proc) { //
NOLINT(runtime/explicit)
+ if (proc_) {
+ proc_->getImpl<T>();
+ }
+ }
+
+ explicit operator bool() const {
+ return proc_ != nullptr;
+ }
+
+ operator core::Processor*() const {
Review Comment:
I'm not sure about this implicit conversion, we could achieve the same with
a dereference + an arrow operator, and that would be explicit.
##########
libminifi/src/core/flow/StructuredConnectionParser.cpp:
##########
@@ -41,8 +41,7 @@ void
StructuredConnectionParser::addFunnelRelationshipToConnection(minifi::Conne
return;
}
- auto& processor_ref = *processor;
- if (typeid(minifi::Funnel) == typeid(processor_ref)) {
+ if (dynamic_cast<minifi::Funnel*>(&processor->getImpl())) {
Review Comment:
I'd prefer typeid over dynamic_cast if inheritance is not a concern.
##########
libminifi/test/libtest/unit/TestBase.h:
##########
@@ -201,13 +201,56 @@ class TempDirectory {
bool is_owner_;
};
+template<typename T>
+class TypedProcessorWrapper {
+ public:
+ TypedProcessorWrapper() = default;
+ TypedProcessorWrapper(core::Processor* proc): proc_(proc) { //
NOLINT(runtime/explicit)
+ if (proc_) {
+ proc_->getImpl<T>();
+ }
+ }
+
+ explicit operator bool() const {
+ return proc_ != nullptr;
+ }
+
+ operator core::Processor*() const {
+ gsl_Assert(proc_);
+ return proc_;
+ }
+
+ T& get() const {
+ gsl_Assert(proc_);
+ return proc_->getImpl<T>();
+ }
+
+ core::Processor* operator->() const {
+ gsl_Assert(proc_);
+ return proc_;
+ }
Review Comment:
Consider adding a matching dereference operator
##########
libminifi/include/core/FlowConfiguration.h:
##########
@@ -84,8 +84,7 @@ class FlowConfiguration : public CoreComponentImpl {
~FlowConfiguration() override;
// Create Processor (Node/Input/Output Port) based on the name
- std::unique_ptr<core::Processor> createProcessor(const std::string &name,
const utils::Identifier &uuid);
- std::unique_ptr<core::Processor> createProcessor(const std::string &name,
const std::string &fullname, const utils::Identifier &uuid);
+ std::unique_ptr<core::Processor> createProcessor(const std::string
&class_short, const std::string &fullclass, const std::string &name, const
utils::Identifier &uuid);
Review Comment:
why do we need two class names?
##########
libminifi/test/libtest/unit/TestBase.h:
##########
@@ -201,13 +201,56 @@ class TempDirectory {
bool is_owner_;
};
+template<typename T>
+class TypedProcessorWrapper {
+ public:
+ TypedProcessorWrapper() = default;
+ TypedProcessorWrapper(core::Processor* proc): proc_(proc) { //
NOLINT(runtime/explicit)
Review Comment:
Please add a comment about why this is okay to be an implicit conversion.
Something like "it's a wrapper over the same object, with no value change".
--
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]
