szaszm commented on a change in pull request #1138:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1138#discussion_r685136268
##########
File path: extensions/aws/controllerservices/AWSCredentialsService.h
##########
@@ -26,6 +26,7 @@
#include "utils/AWSInitializer.h"
#include "core/Resource.h"
+#include "utils/OptionalUtils.h"
Review comment:
This header is not used in the file.
##########
File path: Extensions.md
##########
@@ -16,108 +16,62 @@
To enable all extensions for your platform, you may use -DENABLE_ALL=TRUE OR
select the option to "Enable all Extensions" in the bootstrap script.
[ReadMe](https://github.com/apache/nifi-minifi-cpp/#bootstrapping)
-# Extensions by example
-
-Extensions consist of modules that are conditionally built into your client.
Reasons why you may wish to do this with your modules/processors
-
- - Do not with to make dependencies required or the lack thereof is a
known/expected runtime condition.
- - You wish to allow users to exclude dependencies for a variety of reasons.
-
-# Extensions by example
-We've used HTTP-CURL as the first example. We've taken all libcURL runtime
classes and placed them into an extensions folder
- - /extensions/http-curl
-
-This folder contains a CMakeLists file so that we can conditionally build it.
In the case with libcURL, if the user does not have curl installed OR they
specify -DDISABLE_CURL=true in the cmake build, the extensions will not be
built. In this case, when the extension is not built, C2 REST protocols,
InvokeHTTP, and an HTTP Client implementation will not be included.
-
-Your CMAKE file should build a static library, that will be included into the
run time. This must be added with your conditional to the libminifi CMAKE,
along with a platform specific whole archive inclusion. Note that this will
ensure that despite no direct linkage being found by the compiler, we will
include the code so that we can dynamically find your code.
-
-# Including your extension in the build
-There is a new function that can be used in the root cmake to build and
included your extension. An example is based on the LibArchive extension. The
createExtension function has 8 possible arguments. The first five arguments are
required.
-The first argument specifies the variable controlling the exclusion of this
extension, followed by the variable that
-is used when including it into conditional statements. The third argument is
the pretty name followed by the description of the extension and the extension
directory. The first optional argument is the test directory, which must also
contain a CMakeLists.txt file. The seventh argument can be a conditional
variable that tells us whether or not to add a third party subdirectory
specified by the final extension.
-
-In the lib archive example, we provide all arguments, but your extension may
only need the first five and the the test folder. The seventh and eighth
arguments must be included in tandem.
-
-```cmake
-if ( NOT LibArchive_FOUND OR BUILD_LIBARCHIVE )
- set(BUILD_TP "TRUE")
-endif()
-createExtension(DISABLE_LIBARCHIVE
- ARCHIVE-EXTENSIONS
- "ARCHIVE EXTENSIONS"
- "This Enables libarchive functionality
including MergeContent, CompressContent, and (Un)FocusArchiveEntry"
- "extensions/libarchive"
- "${TEST_DIR}/archive-tests"
- BUILD_TP
- "thirdparty/libarchive-3.3.2")
+# Extension internals
+Extensions are dynamic libraries loaded at runtime by the agent. An extension
makes its
+capabilities (classes) available to the system through registrars.
Registration should happen in source files,
+as otherwise, including another extension's headers would introduce the same
resources in the including extension
+as well, possibly shadowing its own resources.
Review comment:
```suggestion
capabilities (classes) available to the system through registrars.
Registration must happen in source files, not headers.
```
If someone questions it or wants to understand the reason, they can look at
the macro implementations and figure out. Or we can keep the details but move
them to code comments. The improvement would be shorter, easier to follow
documentation.
##########
File path: Extensions.md
##########
@@ -16,108 +16,64 @@
To enable all extensions for your platform, you may use -DENABLE_ALL=TRUE OR
select the option to "Enable all Extensions" in the bootstrap script.
[ReadMe](https://github.com/apache/nifi-minifi-cpp/#bootstrapping)
-# Extensions by example
-
-Extensions consist of modules that are conditionally built into your client.
Reasons why you may wish to do this with your modules/processors
-
- - Do not with to make dependencies required or the lack thereof is a
known/expected runtime condition.
- - You wish to allow users to exclude dependencies for a variety of reasons.
-
-# Extensions by example
-We've used HTTP-CURL as the first example. We've taken all libcURL runtime
classes and placed them into an extensions folder
- - /extensions/http-curl
-
-This folder contains a CMakeLists file so that we can conditionally build it.
In the case with libcURL, if the user does not have curl installed OR they
specify -DDISABLE_CURL=true in the cmake build, the extensions will not be
built. In this case, when the extension is not built, C2 REST protocols,
InvokeHTTP, and an HTTP Client implementation will not be included.
-
-Your CMAKE file should build a static library, that will be included into the
run time. This must be added with your conditional to the libminifi CMAKE,
along with a platform specific whole archive inclusion. Note that this will
ensure that despite no direct linkage being found by the compiler, we will
include the code so that we can dynamically find your code.
-
-# Including your extension in the build
-There is a new function that can be used in the root cmake to build and
included your extension. An example is based on the LibArchive extension. The
createExtension function has 8 possible arguments. The first five arguments are
required.
-The first argument specifies the variable controlling the exclusion of this
extension, followed by the variable that
-is used when including it into conditional statements. The third argument is
the pretty name followed by the description of the extension and the extension
directory. The first optional argument is the test directory, which must also
contain a CMakeLists.txt file. The seventh argument can be a conditional
variable that tells us whether or not to add a third party subdirectory
specified by the final extension.
-
-In the lib archive example, we provide all arguments, but your extension may
only need the first five and the the test folder. The seventh and eighth
arguments must be included in tandem.
-
-```cmake
-if ( NOT LibArchive_FOUND OR BUILD_LIBARCHIVE )
- set(BUILD_TP "TRUE")
-endif()
-createExtension(DISABLE_LIBARCHIVE
- ARCHIVE-EXTENSIONS
- "ARCHIVE EXTENSIONS"
- "This Enables libarchive functionality
including MergeContent, CompressContent, and (Un)FocusArchiveEntry"
- "extensions/libarchive"
- "${TEST_DIR}/archive-tests"
- BUILD_TP
- "thirdparty/libarchive-3.3.2")
+# Extension internals
+Extensions are dynamic libraries loaded at runtime by the agent. An extension
makes its
+capabilities (classes) available to the system through registrars.
+
+``` C++
+// register user-facing classes as
+REGISTER_RESOURCE(InvokeHTTP, "An HTTP client processor which can interact
with a configurable HTTP Endpoint. "
+ "The destination URL and HTTP Method are configurable. FlowFile attributes
are converted to HTTP headers and the "
+ "FlowFile contents are included as the body of the request (if the HTTP
Method is PUT, POST or PATCH).");
+
+// register internal resources as
+REGISTER_INTERNAL_RESOURCE(HTTPClient);
```
-It is advised that you also add your extension to bootstrap.sh as that is the
suggested method of configuring MiNiFi C++
-
-# C bindings
-To find your classes, you must adhere to a dlsym call back that adheres to the
core::ObjectFactory class, like the one below. This object factory will return
a list of classes, that we can instantiate through the class loader mechanism.
Note that since we are including your code directly into our runtime, we will
take care of dlopen and dlsym calls. A map from the class name to the object
factory is kept in memory.
+If you decide to put them in a header file, you better make sure that there
are no dependencies between extensions,
+as the inclusion of such header from extension "A", will force it to be
defined in the including extension "B". This could result
+in shadowing each other's resources. Best to put them in source files.
+
+Some extensions (e.g. `http-curl`) require initialization before use. You need
to subclass `Extension` and let the system know by using `REGISTER_EXTENSION`.
```C++
-class __attribute__((visibility("default"))) HttpCurlObjectFactory : public
core::ObjectFactory {
+class HttpCurlExtension : core::extension::Extension {
public:
Review comment:
I didn't mean exactly gcc constructor/destructor, just similar with our
code. But even that could work by adding a config getter in libminifi, although
I think that would be unnecessary added complexity.
I also think that inheritance is unnecessary added complexity. We don't need
to inherit data or implementation.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c120-use-class-hierarchies-to-represent-concepts-with-inherent-hierarchical-structure-only
##########
File path: extensions/standard-processors/tests/unit/ClassLoaderTests.cpp
##########
@@ -20,6 +20,8 @@
#include "core/Processor.h"
#include "core/ClassLoader.h"
#include "core/yaml/YamlConfiguration.h"
+#include "core/extension/ExtensionManager.h"
+#include "utils/file/FileUtils.h"
Review comment:
Are these necessary?
##########
File path: libminifi/include/utils/file/FileMatcher.h
##########
@@ -0,0 +1,102 @@
+/**
+ * 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 <vector>
+#include <set>
+#include <utility>
+#include <memory>
+
+#include "utils/OptionalUtils.h"
+#include "core/logging/Logger.h"
+
+struct FileMatcherTestAccessor;
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+namespace file {
+
+class FileMatcher {
+ friend struct ::FileMatcherTestAccessor;
+ class FilePattern {
+ FilePattern(std::vector<std::string> directory_segments, std::string
file_pattern, bool excluding)
+ : directory_segments_(std::move(directory_segments)),
+ file_pattern_(std::move(file_pattern)),
+ excluding_(excluding) {}
+
+ public:
+ enum class MatchResult {
+ INCLUDE, // dir/file should be processed according to the pattern
+ EXCLUDE, // dir/file is explicitly rejected by the pattern
+ DONT_CARE // dir/file does not match pattern, do what you may
Review comment:
I would rename DONT_CARE to NOT_MATCHING or similar to convey the
meaning. It was strange reading DONT_CARE in the client code and wondering why
would you not care.
##########
File path: libminifi/include/core/extension/ExtensionManager.h
##########
@@ -0,0 +1,65 @@
+/**
+ * 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 <vector>
+
+#include "core/logging/Logger.h"
+#include "DynamicLibrary.h"
Review comment:
This could be replaced with Module.h
##########
File path: libminifi/include/core/Resource.h
##########
@@ -37,29 +41,64 @@ namespace core {
#define MKSOC(x) #x
#define MAKESTRING(x) MKSOC(x)
+static inline ClassLoader& getClassLoader() {
+#ifdef MODULE_NAME
+ return
ClassLoader::getDefaultClassLoader().getClassLoader(MAKESTRING(MODULE_NAME));
+#else
+ return ClassLoader::getDefaultClassLoader();
+#endif
+}
+
template<class T>
class StaticClassType {
public:
- StaticClassType(const std::string &name, const std::string &description =
"") { // NOLINT
+ StaticClassType(const std::string& name, const std::optional<std::string>&
description, const std::vector<std::string>& construction_names)
+ : name_(name), construction_names_(construction_names) { // NOLINT
Review comment:
Do we still need this NOLINT?
##########
File path: conf/minifi.properties
##########
@@ -20,6 +20,9 @@ nifi.administrative.yield.duration=30 sec
# If a component has no work to do (is "bored"), how long should we wait
before checking again for work?
nifi.bored.yield.duration=100 millis
+# Comma separated path for the extension libraries. Relative path is relative
to the minifi executable.
+nifi.extension.path=../extensions/*
Review comment:
I currently see two usual ways of launching minifi:
- The normal installation, where the MINIFI_HOME is usually /opt/minifi, and
the binary is under MINIFI_HOME/bin
- After compilation, launching the compiled binary in a preexisting
MINIFI_HOME
In the first case, either version is fine. In the second case, we would need
extensions to be installed to build/extensions/ for them to work without extra
effort, or we could load them from MINIFI_HOME/extensions/ in the proposed case.
Another use case that I could see happening in the future is standard linux
packages:
- minifi binary under /usr/bin/minifi
- config files under /etc/minifi
- extensions under /usr/share/minifi/extensions, and I imagine MINIFI_HOME
would be /usr/share/minifi.
I think the default would cover more use cases with a path relative to
MINIFI_HOME, and we could avoid the "../" at the beginning of practically every
path pattern. Given the above points, do you still prefer paths relative to the
binary?
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
