You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/01/04 15:08:29 UTC
[GitHub] [nifi-minifi-cpp] lordgamez opened a new pull request #1232: MINIFICPP-1706 Rework script engine management in ExecuteScript processor
lordgamez opened a new pull request #1232:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1232
https://issues.apache.org/jira/browse/MINIFICPP-1706
------------------------------------------
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:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### 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.
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1232: MINIFICPP-1706 Rework script engine management in ExecuteScript processor
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1232:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1232#discussion_r778803583
##########
File path: extensions/script/ExecuteScript.cpp
##########
@@ -52,6 +52,28 @@ core::Property ExecuteScript::ModuleDirectory("Module Directory", // NOLINT
core::Relationship ExecuteScript::Success("success", "Script successes"); // NOLINT
core::Relationship ExecuteScript::Failure("failure", "Script failures"); // NOLINT
+ScriptEngineFactory::ScriptEngineFactory(core::Relationship& success, core::Relationship& failure, std::shared_ptr<core::logging::Logger> logger)
+ : success_(success),
+ failure_(failure),
+ logger_(logger) {
+}
+
+ScriptEngineQueue::ScriptEngineQueue(uint8_t max_engine_count, ScriptEngineFactory& engine_factory, std::shared_ptr<core::logging::Logger> logger)
+ : max_engine_count_(max_engine_count),
+ engine_factory_(engine_factory),
+ logger_(logger) {
+}
+
+void ScriptEngineQueue::returnScriptEngine(std::shared_ptr<script::ScriptEngine>&& engine) {
+ const std::lock_guard<std::mutex> lock(queue_mutex_);
+ if (engine_queue_.size_approx() < max_engine_count_) {
+ logger_->log_debug("Releasing [%p] script engine", engine.get());
+ engine_queue_.enqueue(engine);
Review comment:
Updated in 268d10998075d418fa7fca9fbd3d318996bda6bd
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1232: MINIFICPP-1706 Rework script engine management in ExecuteScript processor
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1232:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1232#discussion_r778835877
##########
File path: extensions/script/tests/TestExecuteScriptProcessorWithPythonScript.cpp
##########
@@ -27,7 +27,47 @@
#include "processors/GetFile.h"
#include "processors/PutFile.h"
-TEST_CASE("Python: Test Read File", "[executescriptPythonRead]") { // NOLINT
+TEST_CASE("Script engine is not set", "[executescriptMisconfiguration]") {
+ TestController testController;
+ auto plan = testController.createPlan();
+
+ auto executeScript = plan->addProcessor("ExecuteScript", "executeScript");
+
+ plan->setProperty(executeScript, minifi::processors::ExecuteScript::ScriptEngine.getName(), "");
+ plan->setProperty(executeScript, minifi::processors::ExecuteScript::ScriptFile.getName(), "/path/to/script.py");
+
+ REQUIRE_THROWS_AS(testController.runSession(plan, true), minifi::Exception);
+}
+
+TEST_CASE("Neither script body nor script file is set", "[executescriptMisconfiguration]") {
+ TestController testController;
+ auto plan = testController.createPlan();
+
+ auto executeScript = plan->addProcessor("ExecuteScript", "executeScript");
+
+ plan->setProperty(executeScript, minifi::processors::ExecuteScript::ScriptEngine.getName(), "python");
+
+ REQUIRE_THROWS_AS(testController.runSession(plan, true), minifi::Exception);
+}
+
+TEST_CASE("Test both script body and script file set", "[executescriptMisconfiguration]") {
+ TestController testController;
+ auto plan = testController.createPlan();
+
+ auto executeScript = plan->addProcessor("ExecuteScript", "executeScript");
+
+ plan->setProperty(executeScript, minifi::processors::ExecuteScript::ScriptEngine.getName(), "python");
+ plan->setProperty(executeScript, minifi::processors::ExecuteScript::ScriptFile.getName(), "/path/to/script.py");
+ plan->setProperty(executeScript, minifi::processors::ExecuteScript::ScriptBody.getName(), R"(
+ function onTrigger(context, session)
Review comment:
Thanks! Fixed in a1eae0e46cf88802e7e158e235cec6bffe801a0b
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adam-markovics commented on a change in pull request #1232: MINIFICPP-1706 Rework script engine management in ExecuteScript processor
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on a change in pull request #1232:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1232#discussion_r778713003
##########
File path: extensions/script/ExecuteScript.cpp
##########
@@ -89,40 +113,30 @@ void ExecuteScript::onTrigger(const std::shared_ptr<core::ProcessContext> &conte
const std::shared_ptr<core::ProcessSession> &session) {
std::shared_ptr<script::ScriptEngine> engine;
- // Use an existing engine, if one is available
- if (script_engine_q_.try_dequeue(engine)) {
- logger_->log_debug("Using available %s script engine instance", script_engine_);
- } else {
- logger_->log_info("Creating new %s script instance", script_engine_);
- logger_->log_info("Approximately %d %s script instances created for this processor",
- script_engine_q_.size_approx(),
- script_engine_);
-
- if (script_engine_ == "python") {
+ if (script_engine_ == "python") {
#ifdef PYTHON_SUPPORT
- engine = createEngine<python::PythonScriptEngine>();
+ engine = python_script_engine_;
#else
- throw std::runtime_error("Python support is disabled in this build.");
+ throw std::runtime_error("Python support is disabled in this build.");
#endif // PYTHON_SUPPORT
- } else if (script_engine_ == "lua") {
+ } else if (script_engine_ == "lua") {
#ifdef LUA_SUPPORT
- engine = createEngine<lua::LuaScriptEngine>();
+ engine = script_engine_q_->getScriptEngine<lua::LuaScriptEngine>();
#else
- throw std::runtime_error("Lua support is disabled in this build.");
+ throw std::runtime_error("Lua support is disabled in this build.");
#endif // LUA_SUPPORT
- }
-
- if (engine == nullptr) {
- throw std::runtime_error("No script engine available");
- }
-
- if (!script_body_.empty()) {
- engine->eval(script_body_);
- } else if (!script_file_.empty()) {
- engine->evalFile(script_file_);
- } else {
- throw std::runtime_error("Neither Script Body nor Script File is available to execute");
- }
+ }
+
+ if (engine == nullptr) {
+ throw std::runtime_error("No script engine available");
+ }
+
+ if (!script_body_.empty()) {
+ engine->eval(script_body_);
+ } else if (!script_file_.empty()) {
Review comment:
If we have both script_body_ and script_file_ (erroneously), will we just silently execute script_body_, without telling the user about the misconfiguration?
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1232: MINIFICPP-1706 Rework script engine management in ExecuteScript processor
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1232:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1232#discussion_r781187115
##########
File path: extensions/script/ExecuteScript.h
##########
@@ -28,17 +28,95 @@
#include "ScriptEngine.h"
#include "ScriptProcessContext.h"
+#include "utils/Enum.h"
namespace org {
namespace apache {
namespace nifi {
namespace minifi {
+
+#ifdef PYTHON_SUPPORT
+namespace python {
+class PythonScriptEngine;
+}
+#endif // PYTHON_SUPPORT
+
namespace processors {
+class ScriptEngineFactory {
+ public:
+ ScriptEngineFactory(core::Relationship& success, core::Relationship& failure, std::shared_ptr<core::logging::Logger> logger);
+
+ template<typename T>
+ std::shared_ptr<T> createEngine() const {
+ auto engine = std::make_shared<T>();
+
+ engine->bind("log", logger_);
+ engine->bind("REL_SUCCESS", success_);
+ engine->bind("REL_FAILURE", failure_);
+
+ return engine;
+ }
+
+ private:
+ core::Relationship& success_;
+ core::Relationship& failure_;
+ std::shared_ptr<core::logging::Logger> logger_;
+};
+
+class ScriptEngineQueue {
+ public:
+ ScriptEngineQueue(uint8_t max_engine_count, ScriptEngineFactory& engine_factory, std::shared_ptr<core::logging::Logger> logger);
+
+ template<typename T>
+ std::shared_ptr<script::ScriptEngine> getScriptEngine() {
Review comment:
The error of the docker integration test job is related, check it out! https://github.com/apache/nifi-minifi-cpp/runs/4761689961?check_suite_focus=true#step:4:8518
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1232: MINIFICPP-1706 Rework script engine management in ExecuteScript processor
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1232:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1232#discussion_r778803404
##########
File path: extensions/script/ExecuteScript.cpp
##########
@@ -89,40 +113,30 @@ void ExecuteScript::onTrigger(const std::shared_ptr<core::ProcessContext> &conte
const std::shared_ptr<core::ProcessSession> &session) {
std::shared_ptr<script::ScriptEngine> engine;
- // Use an existing engine, if one is available
- if (script_engine_q_.try_dequeue(engine)) {
- logger_->log_debug("Using available %s script engine instance", script_engine_);
- } else {
- logger_->log_info("Creating new %s script instance", script_engine_);
- logger_->log_info("Approximately %d %s script instances created for this processor",
- script_engine_q_.size_approx(),
- script_engine_);
-
- if (script_engine_ == "python") {
+ if (script_engine_ == "python") {
#ifdef PYTHON_SUPPORT
- engine = createEngine<python::PythonScriptEngine>();
+ engine = python_script_engine_;
#else
- throw std::runtime_error("Python support is disabled in this build.");
+ throw std::runtime_error("Python support is disabled in this build.");
#endif // PYTHON_SUPPORT
- } else if (script_engine_ == "lua") {
+ } else if (script_engine_ == "lua") {
#ifdef LUA_SUPPORT
- engine = createEngine<lua::LuaScriptEngine>();
+ engine = script_engine_q_->getScriptEngine<lua::LuaScriptEngine>();
#else
- throw std::runtime_error("Lua support is disabled in this build.");
+ throw std::runtime_error("Lua support is disabled in this build.");
#endif // LUA_SUPPORT
- }
-
- if (engine == nullptr) {
- throw std::runtime_error("No script engine available");
- }
-
- if (!script_body_.empty()) {
- engine->eval(script_body_);
- } else if (!script_file_.empty()) {
- engine->evalFile(script_file_);
- } else {
- throw std::runtime_error("Neither Script Body nor Script File is available to execute");
- }
+ }
+
+ if (engine == nullptr) {
+ throw std::runtime_error("No script engine available");
+ }
+
+ if (!script_body_.empty()) {
+ engine->eval(script_body_);
+ } else if (!script_file_.empty()) {
Review comment:
Good point, I added additional checks and tests for misconfiguration use cases in fb9b4e78d3762de139ca065b3044f572ca399a14
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adam-markovics commented on a change in pull request #1232: MINIFICPP-1706 Rework script engine management in ExecuteScript processor
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on a change in pull request #1232:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1232#discussion_r778717351
##########
File path: extensions/script/ExecuteScript.cpp
##########
@@ -52,6 +52,28 @@ core::Property ExecuteScript::ModuleDirectory("Module Directory", // NOLINT
core::Relationship ExecuteScript::Success("success", "Script successes"); // NOLINT
core::Relationship ExecuteScript::Failure("failure", "Script failures"); // NOLINT
+ScriptEngineFactory::ScriptEngineFactory(core::Relationship& success, core::Relationship& failure, std::shared_ptr<core::logging::Logger> logger)
+ : success_(success),
+ failure_(failure),
+ logger_(logger) {
+}
+
+ScriptEngineQueue::ScriptEngineQueue(uint8_t max_engine_count, ScriptEngineFactory& engine_factory, std::shared_ptr<core::logging::Logger> logger)
+ : max_engine_count_(max_engine_count),
+ engine_factory_(engine_factory),
+ logger_(logger) {
+}
+
+void ScriptEngineQueue::returnScriptEngine(std::shared_ptr<script::ScriptEngine>&& engine) {
+ const std::lock_guard<std::mutex> lock(queue_mutex_);
+ if (engine_queue_.size_approx() < max_engine_count_) {
+ logger_->log_debug("Releasing [%p] script engine", engine.get());
+ engine_queue_.enqueue(engine);
Review comment:
std::move could be used
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1232: MINIFICPP-1706 Rework script engine management in ExecuteScript processor
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1232:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1232#discussion_r781857820
##########
File path: extensions/script/tests/CMakeLists.txt
##########
@@ -30,6 +30,7 @@ endif()
if (ENABLE_LUA_SCRIPTING)
file(GLOB EXECUTESCRIPT_LUA_TESTS "TestExecuteScriptProcessorWithLuaScript.cpp" "LuaScriptEngineTests.cpp")
+ include_directories(${LUA_INCLUDE_DIR})
Review comment:
Thanks for the tip, I wasn't sure if there's a better way to have those includes available for all test targets without duplication, but making it PUBLIC seems to work, changed it in 5914deef48db52f59a26dcf432e2af8262841001
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1232: MINIFICPP-1706 Rework script engine management in ExecuteScript processor
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1232:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1232#discussion_r781663088
##########
File path: extensions/script/tests/CMakeLists.txt
##########
@@ -30,6 +30,7 @@ endif()
if (ENABLE_LUA_SCRIPTING)
file(GLOB EXECUTESCRIPT_LUA_TESTS "TestExecuteScriptProcessorWithLuaScript.cpp" "LuaScriptEngineTests.cpp")
+ include_directories(${LUA_INCLUDE_DIR})
Review comment:
Don't use `include_directories`, because it's directory-based and global. Specify them for the appropriate targets instead, with `target_include_directories`. You may want to specify this one as PUBLIC on the sol2 or the minifi-script-extensions target.
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1232: MINIFICPP-1706 Rework script engine management in ExecuteScript processor
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1232:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1232#discussion_r778235809
##########
File path: extensions/script/ExecuteScript.h
##########
@@ -28,17 +28,83 @@
#include "ScriptEngine.h"
#include "ScriptProcessContext.h"
+#include "python/PythonScriptEngine.h"
Review comment:
I think this wont compile if we disable python and enable lua
e.g.
`-DDISABLE_PYTHON_SCRIPTING=ON -DENABLE_SCRIPTING=ON -DENABLE_LUA_SCRIPTING=ON`
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1232: MINIFICPP-1706 Rework script engine management in ExecuteScript processor
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1232:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1232#discussion_r781663088
##########
File path: extensions/script/tests/CMakeLists.txt
##########
@@ -30,6 +30,7 @@ endif()
if (ENABLE_LUA_SCRIPTING)
file(GLOB EXECUTESCRIPT_LUA_TESTS "TestExecuteScriptProcessorWithLuaScript.cpp" "LuaScriptEngineTests.cpp")
+ include_directories(${LUA_INCLUDE_DIR})
Review comment:
Don't use `include_directories`, because it's directory-based and global. Specify them for the appropriate targets instead. You may want to specify this one as PUBLIC on the sol2 or the minifi-script-extensions target.
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni closed pull request #1232: MINIFICPP-1706 Rework script engine management in ExecuteScript processor
Posted by GitBox <gi...@apache.org>.
adamdebreceni closed pull request #1232:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1232
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1232: MINIFICPP-1706 Rework script engine management in ExecuteScript processor
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1232:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1232#discussion_r781149292
##########
File path: extensions/script/ExecuteScript.h
##########
@@ -28,17 +28,95 @@
#include "ScriptEngine.h"
#include "ScriptProcessContext.h"
+#include "utils/Enum.h"
namespace org {
namespace apache {
namespace nifi {
namespace minifi {
+
+#ifdef PYTHON_SUPPORT
+namespace python {
+class PythonScriptEngine;
+}
+#endif // PYTHON_SUPPORT
+
namespace processors {
+class ScriptEngineFactory {
+ public:
+ ScriptEngineFactory(core::Relationship& success, core::Relationship& failure, std::shared_ptr<core::logging::Logger> logger);
+
+ template<typename T>
+ std::shared_ptr<T> createEngine() const {
+ auto engine = std::make_shared<T>();
+
+ engine->bind("log", logger_);
+ engine->bind("REL_SUCCESS", success_);
+ engine->bind("REL_FAILURE", failure_);
+
+ return engine;
+ }
+
+ private:
+ core::Relationship& success_;
+ core::Relationship& failure_;
+ std::shared_ptr<core::logging::Logger> logger_;
+};
+
+class ScriptEngineQueue {
+ public:
+ ScriptEngineQueue(uint8_t max_engine_count, ScriptEngineFactory& engine_factory, std::shared_ptr<core::logging::Logger> logger);
+
+ template<typename T>
+ std::shared_ptr<script::ScriptEngine> getScriptEngine() {
Review comment:
How is it enforced that the returned script engine is of type `T`, especially if it's taken out of the queue? It looks like the queue can contain any kind of script engine and this function just takes out whatever is at the end.
##########
File path: extensions/script/ExecuteScript.h
##########
@@ -28,17 +28,95 @@
#include "ScriptEngine.h"
#include "ScriptProcessContext.h"
+#include "utils/Enum.h"
namespace org {
namespace apache {
namespace nifi {
namespace minifi {
+
+#ifdef PYTHON_SUPPORT
+namespace python {
+class PythonScriptEngine;
+}
+#endif // PYTHON_SUPPORT
+
namespace processors {
+class ScriptEngineFactory {
+ public:
+ ScriptEngineFactory(core::Relationship& success, core::Relationship& failure, std::shared_ptr<core::logging::Logger> logger);
+
+ template<typename T>
+ std::shared_ptr<T> createEngine() const {
+ auto engine = std::make_shared<T>();
Review comment:
I think it would make sense to add some restriction, e.g. that `T` is derived from `ScriptEngine`.
```suggestion
template<typename T>
std::enable_if_t<std::is_base_of_v<script::ScriptEngine, T>, std::shared_ptr<T>> createEngine() const {
auto engine = std::make_shared<T>();
```
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1232: MINIFICPP-1706 Rework script engine management in ExecuteScript processor
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1232:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1232#discussion_r781226221
##########
File path: extensions/script/ExecuteScript.h
##########
@@ -28,17 +28,95 @@
#include "ScriptEngine.h"
#include "ScriptProcessContext.h"
+#include "utils/Enum.h"
namespace org {
namespace apache {
namespace nifi {
namespace minifi {
+
+#ifdef PYTHON_SUPPORT
+namespace python {
+class PythonScriptEngine;
+}
+#endif // PYTHON_SUPPORT
+
namespace processors {
+class ScriptEngineFactory {
+ public:
+ ScriptEngineFactory(core::Relationship& success, core::Relationship& failure, std::shared_ptr<core::logging::Logger> logger);
+
+ template<typename T>
+ std::shared_ptr<T> createEngine() const {
+ auto engine = std::make_shared<T>();
+
+ engine->bind("log", logger_);
+ engine->bind("REL_SUCCESS", success_);
+ engine->bind("REL_FAILURE", failure_);
+
+ return engine;
+ }
+
+ private:
+ core::Relationship& success_;
+ core::Relationship& failure_;
+ std::shared_ptr<core::logging::Logger> logger_;
+};
+
+class ScriptEngineQueue {
+ public:
+ ScriptEngineQueue(uint8_t max_engine_count, ScriptEngineFactory& engine_factory, std::shared_ptr<core::logging::Logger> logger);
+
+ template<typename T>
+ std::shared_ptr<script::ScriptEngine> getScriptEngine() {
Review comment:
My bad I didn't have the LUA_SUPPORT flag enabled when compiled locally, the compilation issues should be fixed in 9c96d09addaa6516382a2d171f002e14f4151099
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1232: MINIFICPP-1706 Rework script engine management in ExecuteScript processor
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1232:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1232#discussion_r778671698
##########
File path: extensions/script/ExecuteScript.h
##########
@@ -28,17 +28,83 @@
#include "ScriptEngine.h"
#include "ScriptProcessContext.h"
+#include "python/PythonScriptEngine.h"
Review comment:
Good catch! I added additional compile flag checks to be consistent with the previous Lua and Python support checks in 108a39ccc89cdf6d229b19f5a80545ecb925c993
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1232: MINIFICPP-1706 Rework script engine management in ExecuteScript processor
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1232:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1232#discussion_r781179483
##########
File path: extensions/script/ExecuteScript.h
##########
@@ -28,17 +28,95 @@
#include "ScriptEngine.h"
#include "ScriptProcessContext.h"
+#include "utils/Enum.h"
namespace org {
namespace apache {
namespace nifi {
namespace minifi {
+
+#ifdef PYTHON_SUPPORT
+namespace python {
+class PythonScriptEngine;
+}
+#endif // PYTHON_SUPPORT
+
namespace processors {
+class ScriptEngineFactory {
+ public:
+ ScriptEngineFactory(core::Relationship& success, core::Relationship& failure, std::shared_ptr<core::logging::Logger> logger);
+
+ template<typename T>
+ std::shared_ptr<T> createEngine() const {
+ auto engine = std::make_shared<T>();
+
+ engine->bind("log", logger_);
+ engine->bind("REL_SUCCESS", success_);
+ engine->bind("REL_FAILURE", failure_);
+
+ return engine;
+ }
+
+ private:
+ core::Relationship& success_;
+ core::Relationship& failure_;
+ std::shared_ptr<core::logging::Logger> logger_;
+};
+
+class ScriptEngineQueue {
+ public:
+ ScriptEngineQueue(uint8_t max_engine_count, ScriptEngineFactory& engine_factory, std::shared_ptr<core::logging::Logger> logger);
+
+ template<typename T>
+ std::shared_ptr<script::ScriptEngine> getScriptEngine() {
Review comment:
Good point, there is no point of having different types of script engines in a queue. Added a class level template parameter for the engine type and similar type restriction to `createEngine` in e1583ef65af13edcd0d5b07da6eee5ea84ba3cbd
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1232: MINIFICPP-1706 Rework script engine management in ExecuteScript processor
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1232:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1232#discussion_r781178489
##########
File path: extensions/script/ExecuteScript.h
##########
@@ -28,17 +28,95 @@
#include "ScriptEngine.h"
#include "ScriptProcessContext.h"
+#include "utils/Enum.h"
namespace org {
namespace apache {
namespace nifi {
namespace minifi {
+
+#ifdef PYTHON_SUPPORT
+namespace python {
+class PythonScriptEngine;
+}
+#endif // PYTHON_SUPPORT
+
namespace processors {
+class ScriptEngineFactory {
+ public:
+ ScriptEngineFactory(core::Relationship& success, core::Relationship& failure, std::shared_ptr<core::logging::Logger> logger);
+
+ template<typename T>
+ std::shared_ptr<T> createEngine() const {
+ auto engine = std::make_shared<T>();
Review comment:
Good idea, added restriction in e1583ef65af13edcd0d5b07da6eee5ea84ba3cbd
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adam-markovics commented on a change in pull request #1232: MINIFICPP-1706 Rework script engine management in ExecuteScript processor
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on a change in pull request #1232:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1232#discussion_r778829451
##########
File path: extensions/script/tests/TestExecuteScriptProcessorWithPythonScript.cpp
##########
@@ -27,7 +27,47 @@
#include "processors/GetFile.h"
#include "processors/PutFile.h"
-TEST_CASE("Python: Test Read File", "[executescriptPythonRead]") { // NOLINT
+TEST_CASE("Script engine is not set", "[executescriptMisconfiguration]") {
+ TestController testController;
+ auto plan = testController.createPlan();
+
+ auto executeScript = plan->addProcessor("ExecuteScript", "executeScript");
+
+ plan->setProperty(executeScript, minifi::processors::ExecuteScript::ScriptEngine.getName(), "");
+ plan->setProperty(executeScript, minifi::processors::ExecuteScript::ScriptFile.getName(), "/path/to/script.py");
+
+ REQUIRE_THROWS_AS(testController.runSession(plan, true), minifi::Exception);
+}
+
+TEST_CASE("Neither script body nor script file is set", "[executescriptMisconfiguration]") {
+ TestController testController;
+ auto plan = testController.createPlan();
+
+ auto executeScript = plan->addProcessor("ExecuteScript", "executeScript");
+
+ plan->setProperty(executeScript, minifi::processors::ExecuteScript::ScriptEngine.getName(), "python");
+
+ REQUIRE_THROWS_AS(testController.runSession(plan, true), minifi::Exception);
+}
+
+TEST_CASE("Test both script body and script file set", "[executescriptMisconfiguration]") {
+ TestController testController;
+ auto plan = testController.createPlan();
+
+ auto executeScript = plan->addProcessor("ExecuteScript", "executeScript");
+
+ plan->setProperty(executeScript, minifi::processors::ExecuteScript::ScriptEngine.getName(), "python");
+ plan->setProperty(executeScript, minifi::processors::ExecuteScript::ScriptFile.getName(), "/path/to/script.py");
+ plan->setProperty(executeScript, minifi::processors::ExecuteScript::ScriptBody.getName(), R"(
+ function onTrigger(context, session)
Review comment:
I think Python code would suit better here. Just to make sure the exception is not coming because of the wrong code syntax.
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org