You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by arpadboda <gi...@git.apache.org> on 2018/10/30 16:20:23 UTC
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
GitHub user arpadboda opened a pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432
MINIFICPP-648 - add processor and add processor with linkage nomencla…
…ture is confusing
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 master)?
- [ ] 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 travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/arpadboda/nifi-minifi-cpp MINIFICPP-648
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi-minifi-cpp/pull/432.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #432
----
commit c0e679d2cf2db5b7920b23eeed142b450270e2a1
Author: Arpad Boda <ab...@...>
Date: 2018-10-26T12:50:55Z
MINIFICPP-648 - add processor and add processor with linkage nomenclature is confusing
----
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231867815
--- Diff: nanofi/src/api/nanofi.cpp ---
@@ -323,55 +360,45 @@ int transmit_flowfile(flow_file_record *ff, nifi_instance *instance) {
flow * create_new_flow(nifi_instance * instance) {
auto minifi_instance_ref = static_cast<minifi::Instance*>(instance->instance_ptr);
- flow *new_flow = (flow*) malloc(sizeof(flow));
-
- auto execution_plan = new ExecutionPlan(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
-
- new_flow->plan = execution_plan;
-
- return new_flow;
+ return new flow(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
}
flow *create_flow(nifi_instance *instance, const char *first_processor) {
if (nullptr == instance || nullptr == instance->instance_ptr) {
return nullptr;
}
auto minifi_instance_ref = static_cast<minifi::Instance*>(instance->instance_ptr);
- flow *new_flow = (flow*) malloc(sizeof(flow));
-
- auto execution_plan = new ExecutionPlan(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
- new_flow->plan = execution_plan;
+ flow *new_flow = new flow(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
if (first_processor != nullptr && strlen(first_processor) > 0) {
// automatically adds it with success
- execution_plan->addProcessor(first_processor, first_processor);
--- End diff --
Combined C and C++ implies we're moving more toward C++. If that were the case this API would be drastically different. We should move in the other direction since C++ functions as a temporary interconnect.
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231878067
--- Diff: nanofi/include/core/cxxstructs.h ---
@@ -0,0 +1,41 @@
+/**
+ *
+ * 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.
+ */
+
+#ifndef NIFI_MINIFI_CPP_CXXSTRUCTS_H
+#define NIFI_MINIFI_CPP_CXXSTRUCTS_H
+
+#include "cstructs.h"
+#include "cxx/Plan.h"
+
--- End diff --
I don't think is moving that way, just simplifies the current structure.
There is no change in API, no change in the background C++ classes, just removed the C struct we used to interconnect.
In case the user doesn't know anything about the internal layout (the C struct only contains a void*), creating a struct just adds an extra hop in the memory, but no value.
With the change the user only sees these structures declared, but not defined and gets pointers. The reason we can do it is that the API user doesn't need to know the size or the internal representation of the object in case only gets and passes pointers.
We can change the internal layout as we want without impacting the API anyhow. Even C structs can be added later, that's fine.
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231868999
--- Diff: nanofi/src/cxx/Plan.cpp ---
@@ -162,6 +153,21 @@ bool ExecutionPlan::runNextProcessor(std::function<void(const std::shared_ptr<co
}
std::shared_ptr<core::ProcessSession> current_session = std::make_shared<core::ProcessSession>(context);
process_sessions_.push_back(current_session);
+ if (input_ff != nullptr) {
+ auto content_repo = static_cast<std::shared_ptr<minifi::core::ContentRepository>*>(input_ff->crp);
+ std::shared_ptr<minifi::ResourceClaim> claim = std::make_shared<minifi::ResourceClaim>(input_ff->contentLocation, *content_repo);
+ auto stream = (*content_repo)->read(claim);
+ std::shared_ptr<minifi::FlowFileRecord> flowFile = std::static_pointer_cast<minifi::FlowFileRecord>(current_session->create());
+ auto map = static_cast<std::map<std::string, std::string>*>(input_ff->attributes);
+ if (map) {
+ for (const auto& attr: *map) {
+ flowFile->setAttribute(attr.first, attr.second);
+ }
+ }
+ current_session->importFrom(*stream, flowFile);
+ current_session->transfer(flowFile, core::Relationship("success", "success"));
+ relationships_[relationships_.size()-1]->put(std::static_pointer_cast<core::FlowFile>(flowFile));
--- End diff --
is there a safeguard to ensure a relationship exists?
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231879717
--- Diff: nanofi/src/api/nanofi.cpp ---
@@ -89,6 +93,39 @@ nifi_instance *create_instance(const char *url, nifi_port *port) {
return instance;
}
+standalone_processor *create_processor(const char *name) {
+ static int proc_counter = 0;
+ auto ptr = ExecutionPlan::createProcessor(name, name);
+ if (!ptr) {
+ return nullptr;
+ }
+ if (standalone_instance == nullptr) {
+ nifi_port port;
--- End diff --
Created with dummy value. Do you think having a valid input port is valid usecase for an instance we don't even expose? (As we use it to "store" standalone processors)
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r229379233
--- Diff: libminifi/src/capi/Plan.cpp ---
@@ -23,19 +23,8 @@
#include <set>
#include <string>
-bool intToFailureStragey(int in, FailureStrategy *out) {
--- End diff --
This was just forgotten in the previously merged PR, the declaration was already removed from the header.
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231884707
--- Diff: nanofi/src/api/nanofi.cpp ---
@@ -323,55 +360,45 @@ int transmit_flowfile(flow_file_record *ff, nifi_instance *instance) {
flow * create_new_flow(nifi_instance * instance) {
auto minifi_instance_ref = static_cast<minifi::Instance*>(instance->instance_ptr);
- flow *new_flow = (flow*) malloc(sizeof(flow));
-
- auto execution_plan = new ExecutionPlan(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
-
- new_flow->plan = execution_plan;
-
- return new_flow;
+ return new flow(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
--- End diff --
Multiple thins to add there:
- Please note that this is not an introduced "new" call, just the one 5 lines above.
- We can change this to use malloc and placement new, in which case it ensures that free only causes some leak, but no crash.
- I don't think this approach is sustainable in long term. To properly free structures the users would be required to know the internal layout of the structure, which is not going to happen and it would lead to very complex free strategies in case of shared ownerships. Moreover I don't think that the approach "feel free to free, it's just a leak" is the way forward. One of the reasons I only left declarations in cstructs.h is that freeing something you don't even know should make you raise concerns. What I would really push is the user freeing up all the stuff he allocated and we provide free functions for every structure that the API may return. Don't free what you don't allocate is quite clear and well defines responsibilities. To go even further here, letting the users free things might be a painful barrier in case we plan to change the internal representation, which they shouldn't be aware of anyway.
In nutshell, combining free with placement new is good enough to avoid immediate crashes, but we should try to highlight that the structures allocated by the API should also be freed by that.
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231865428
--- Diff: nanofi/include/cxx/Plan.h ---
@@ -133,8 +133,29 @@ class ExecutionPlan {
next_ff_ = ptr;
}
+ bool hasProcessor() {
+ return !processor_queue_.empty();
+ }
+
static std::shared_ptr<core::Processor> createProcessor(const std::string &processor_name, const std::string &name);
+ static std::shared_ptr<ExecutionPlan> getPlan(const std::string& uuid) {
+ auto it = proc_plan_map_.find(uuid);
+ return it != proc_plan_map_.end() ? it->second : nullptr;
+ }
+
+ static void addProcWithPlan(const std::string& uuid, std::shared_ptr<ExecutionPlan> plan) {
--- End diff --
addProcessorWithPlan
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r230800455
--- Diff: libminifi/CMakeLists.txt ---
@@ -141,11 +141,14 @@ endif()
SET (LIBMINIFI core-minifi PARENT_SCOPE)
if (ENABLE_PYTHON)
-if (NOT APPLE)
#### shared
add_library(core-minifi-shared SHARED ${SOURCES})
-target_link_libraries(core-minifi-shared ${CMAKE_DL_LIBS} uuid-shared yaml-cpp)
+if (APPLE)
--- End diff --
Indeed, it ended up with double definition errors. However works fine in Linux.
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r230803774
--- Diff: libminifi/CMakeLists.txt ---
@@ -141,11 +141,14 @@ endif()
SET (LIBMINIFI core-minifi PARENT_SCOPE)
if (ENABLE_PYTHON)
-if (NOT APPLE)
#### shared
add_library(core-minifi-shared SHARED ${SOURCES})
-target_link_libraries(core-minifi-shared ${CMAKE_DL_LIBS} uuid-shared yaml-cpp)
+if (APPLE)
--- End diff --
Indeed -- https://issues.apache.org/jira/browse/MINIFICPP-660 already exists. I've pre-empted that in favor of CoAP, but should get back to that soon. Python is kind of WIP. I do have some tests from that branch that cover what you are talking about. I'll try and re-surface them as soon as I get away from CoAP.
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi-minifi-cpp/pull/432
---
[GitHub] nifi-minifi-cpp issue #432: MINIFICPP-648 - add processor and add processor ...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/432
@arpadboda is this good? I'm good with this otherwise.
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r230801807
--- Diff: libminifi/CMakeLists.txt ---
@@ -141,11 +141,14 @@ endif()
SET (LIBMINIFI core-minifi PARENT_SCOPE)
if (ENABLE_PYTHON)
-if (NOT APPLE)
#### shared
add_library(core-minifi-shared SHARED ${SOURCES})
-target_link_libraries(core-minifi-shared ${CMAKE_DL_LIBS} uuid-shared yaml-cpp)
+if (APPLE)
--- End diff --
I strongly agree, but in that case Python related tests (the ones that don't transport, just create instance, processor, flow files) could also be added to cover that.
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231876679
--- Diff: nanofi/include/cxx/CallbackProcessor.h ---
@@ -65,7 +65,7 @@ class CallbackProcessor : public core::Processor {
public:
- void setCallback(void *obj,std::function<void(processor_session*)> ontrigger_callback) {
+ void setCallback(void *obj,std::function<void(core::ProcessSession*)> ontrigger_callback) {
--- End diff --
Yes, tested and works.
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r230799918
--- Diff: libminifi/CMakeLists.txt ---
@@ -141,11 +141,14 @@ endif()
SET (LIBMINIFI core-minifi PARENT_SCOPE)
if (ENABLE_PYTHON)
-if (NOT APPLE)
#### shared
add_library(core-minifi-shared SHARED ${SOURCES})
-target_link_libraries(core-minifi-shared ${CMAKE_DL_LIBS} uuid-shared yaml-cpp)
+if (APPLE)
--- End diff --
I think uuid-shared is already linked elsewhere, so I guess it was just unnecessary?
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231867605
--- Diff: nanofi/src/api/nanofi.cpp ---
@@ -323,55 +360,45 @@ int transmit_flowfile(flow_file_record *ff, nifi_instance *instance) {
flow * create_new_flow(nifi_instance * instance) {
auto minifi_instance_ref = static_cast<minifi::Instance*>(instance->instance_ptr);
- flow *new_flow = (flow*) malloc(sizeof(flow));
-
- auto execution_plan = new ExecutionPlan(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
-
- new_flow->plan = execution_plan;
-
- return new_flow;
+ return new flow(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
}
flow *create_flow(nifi_instance *instance, const char *first_processor) {
if (nullptr == instance || nullptr == instance->instance_ptr) {
return nullptr;
}
auto minifi_instance_ref = static_cast<minifi::Instance*>(instance->instance_ptr);
- flow *new_flow = (flow*) malloc(sizeof(flow));
-
- auto execution_plan = new ExecutionPlan(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
- new_flow->plan = execution_plan;
+ flow *new_flow = new flow(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
--- End diff --
same malloc / new
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231886702
--- Diff: nanofi/src/cxx/Plan.cpp ---
@@ -162,6 +153,21 @@ bool ExecutionPlan::runNextProcessor(std::function<void(const std::shared_ptr<co
}
std::shared_ptr<core::ProcessSession> current_session = std::make_shared<core::ProcessSession>(context);
process_sessions_.push_back(current_session);
+ if (input_ff != nullptr) {
+ auto content_repo = static_cast<std::shared_ptr<minifi::core::ContentRepository>*>(input_ff->crp);
+ std::shared_ptr<minifi::ResourceClaim> claim = std::make_shared<minifi::ResourceClaim>(input_ff->contentLocation, *content_repo);
+ auto stream = (*content_repo)->read(claim);
+ std::shared_ptr<minifi::FlowFileRecord> flowFile = std::static_pointer_cast<minifi::FlowFileRecord>(current_session->create());
+ auto map = static_cast<std::map<std::string, std::string>*>(input_ff->attributes);
+ if (map) {
+ for (const auto& attr: *map) {
+ flowFile->setAttribute(attr.first, attr.second);
+ }
+ }
+ current_session->importFrom(*stream, flowFile);
+ current_session->transfer(flowFile, core::Relationship("success", "success"));
+ relationships_[relationships_.size()-1]->put(std::static_pointer_cast<core::FlowFile>(flowFile));
--- End diff --
The finalise() call introduces a new relationship anyway, that's the only one we expect to have in case of standalone processors.
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231865178
--- Diff: nanofi/include/cxx/CallbackProcessor.h ---
@@ -65,7 +65,7 @@ class CallbackProcessor : public core::Processor {
public:
- void setCallback(void *obj,std::function<void(processor_session*)> ontrigger_callback) {
+ void setCallback(void *obj,std::function<void(core::ProcessSession*)> ontrigger_callback) {
--- End diff --
It's been a while since I've looked at that work, but did the python bindings call this correctly when running python processors?
---
[GitHub] nifi-minifi-cpp issue #432: MINIFICPP-648 - add processor and add processor ...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/432
Incomplete as the functionality to use a standalone processor with existing data is not yet implemented, however the already done part is not planned to be changed, so happy to receive review comments.
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231865773
--- Diff: nanofi/src/api/nanofi.cpp ---
@@ -89,6 +93,39 @@ nifi_instance *create_instance(const char *url, nifi_port *port) {
return instance;
}
+standalone_processor *create_processor(const char *name) {
+ static int proc_counter = 0;
+ auto ptr = ExecutionPlan::createProcessor(name, name);
+ if (!ptr) {
+ return nullptr;
+ }
+ if (standalone_instance == nullptr) {
+ nifi_port port;
--- End diff --
this is an input port identifier.
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231865240
--- Diff: nanofi/include/cxx/Plan.h ---
@@ -44,7 +44,7 @@
#include "core/ProcessSession.h"
#include "core/ProcessorNode.h"
#include "core/reporting/SiteToSiteProvenanceReportingTask.h"
-#include "core/cstructs.h"
--- End diff --
Should remove the rem'd line.
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231880575
--- Diff: nanofi/include/cxx/Plan.h ---
@@ -44,7 +44,7 @@
#include "core/ProcessSession.h"
#include "core/ProcessorNode.h"
#include "core/reporting/SiteToSiteProvenanceReportingTask.h"
-#include "core/cstructs.h"
--- End diff --
Thanks, done.
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r232751327
--- Diff: nanofi/src/api/nanofi.cpp ---
@@ -323,55 +360,45 @@ int transmit_flowfile(flow_file_record *ff, nifi_instance *instance) {
flow * create_new_flow(nifi_instance * instance) {
auto minifi_instance_ref = static_cast<minifi::Instance*>(instance->instance_ptr);
- flow *new_flow = (flow*) malloc(sizeof(flow));
-
- auto execution_plan = new ExecutionPlan(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
-
- new_flow->plan = execution_plan;
-
- return new_flow;
+ return new flow(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
}
flow *create_flow(nifi_instance *instance, const char *first_processor) {
if (nullptr == instance || nullptr == instance->instance_ptr) {
return nullptr;
}
auto minifi_instance_ref = static_cast<minifi::Instance*>(instance->instance_ptr);
- flow *new_flow = (flow*) malloc(sizeof(flow));
-
- auto execution_plan = new ExecutionPlan(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
- new_flow->plan = execution_plan;
+ flow *new_flow = new flow(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
if (first_processor != nullptr && strlen(first_processor) > 0) {
// automatically adds it with success
- execution_plan->addProcessor(first_processor, first_processor);
--- End diff --
What are you basing the statement, "it's now faster than it was?" What profiler did you use to gather this, and what benefit does that have over readability concerns?
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r232749999
--- Diff: nanofi/include/core/cxxstructs.h ---
@@ -0,0 +1,41 @@
+/**
+ *
+ * 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.
+ */
+
+#ifndef NIFI_MINIFI_CPP_CXXSTRUCTS_H
+#define NIFI_MINIFI_CPP_CXXSTRUCTS_H
+
+#include "cstructs.h"
+#include "cxx/Plan.h"
+
--- End diff --
What do you mean by "I don't think is moving that way, just simplifies the current structure." ? You don't see it moving away from C++?
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231864832
--- Diff: nanofi/include/core/cxxstructs.h ---
@@ -0,0 +1,41 @@
+/**
+ *
+ * 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.
+ */
+
+#ifndef NIFI_MINIFI_CPP_CXXSTRUCTS_H
+#define NIFI_MINIFI_CPP_CXXSTRUCTS_H
+
+#include "cstructs.h"
+#include "cxx/Plan.h"
+
--- End diff --
A lot of this works seems counter to tickets to move away from C++. These structs were created extending C++ classes. and the use of cxx structs seems unnecessary as a result.
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231202899
--- Diff: nanofi/src/cxx/Plan.cpp ---
@@ -162,6 +153,21 @@ bool ExecutionPlan::runNextProcessor(std::function<void(const std::shared_ptr<co
}
std::shared_ptr<core::ProcessSession> current_session = std::make_shared<core::ProcessSession>(context);
process_sessions_.push_back(current_session);
+ if (input_ff != nullptr) {
+ auto content_repo = static_cast<std::shared_ptr<minifi::core::ContentRepository>*>(input_ff->crp);
+ std::shared_ptr<minifi::ResourceClaim> claim = std::make_shared<minifi::ResourceClaim>(input_ff->contentLocation, *content_repo);
+ auto stream = (*content_repo)->read(claim);
--- End diff --
This copies the content of the incoming flow file.
Naturally this leaves space for improvement, in case the content repos of the current flow and the incoming flow file are the same, the copy is needless.
Given the size of the change I would prefer to do it later.
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r230801158
--- Diff: libminifi/CMakeLists.txt ---
@@ -141,11 +141,14 @@ endif()
SET (LIBMINIFI core-minifi PARENT_SCOPE)
if (ENABLE_PYTHON)
-if (NOT APPLE)
#### shared
add_library(core-minifi-shared SHARED ${SOURCES})
-target_link_libraries(core-minifi-shared ${CMAKE_DL_LIBS} uuid-shared yaml-cpp)
+if (APPLE)
--- End diff --
Hmm. Maybe we should add -DENABLE_PYTHON to our travis builds
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231889164
--- Diff: nanofi/include/cxx/Plan.h ---
@@ -133,8 +133,29 @@ class ExecutionPlan {
next_ff_ = ptr;
}
+ bool hasProcessor() {
+ return !processor_queue_.empty();
+ }
+
static std::shared_ptr<core::Processor> createProcessor(const std::string &processor_name, const std::string &name);
+ static std::shared_ptr<ExecutionPlan> getPlan(const std::string& uuid) {
+ auto it = proc_plan_map_.find(uuid);
+ return it != proc_plan_map_.end() ? it->second : nullptr;
+ }
+
+ static void addProcWithPlan(const std::string& uuid, std::shared_ptr<ExecutionPlan> plan) {
--- End diff --
Renamed
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231886506
--- Diff: nanofi/src/api/nanofi.cpp ---
@@ -323,55 +360,45 @@ int transmit_flowfile(flow_file_record *ff, nifi_instance *instance) {
flow * create_new_flow(nifi_instance * instance) {
auto minifi_instance_ref = static_cast<minifi::Instance*>(instance->instance_ptr);
- flow *new_flow = (flow*) malloc(sizeof(flow));
-
- auto execution_plan = new ExecutionPlan(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
-
- new_flow->plan = execution_plan;
-
- return new_flow;
+ return new flow(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
}
flow *create_flow(nifi_instance *instance, const char *first_processor) {
if (nullptr == instance || nullptr == instance->instance_ptr) {
return nullptr;
}
auto minifi_instance_ref = static_cast<minifi::Instance*>(instance->instance_ptr);
- flow *new_flow = (flow*) malloc(sizeof(flow));
-
- auto execution_plan = new ExecutionPlan(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
- new_flow->plan = execution_plan;
+ flow *new_flow = new flow(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
if (first_processor != nullptr && strlen(first_processor) > 0) {
// automatically adds it with success
- execution_plan->addProcessor(first_processor, first_processor);
--- End diff --
I guess that only declaring structures via the API leaves us the freedom to change the internal representation anyhow without breaking it. On the other side it's now faster than it was.
---
[GitHub] nifi-minifi-cpp issue #432: MINIFICPP-648 - add processor and add processor ...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/432
>
>
> @arpadboda is this good? I'm good with this otherwise.
Now it is.
---
[GitHub] nifi-minifi-cpp pull request #432: MINIFICPP-648 - add processor and add pro...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231866668
--- Diff: nanofi/src/api/nanofi.cpp ---
@@ -323,55 +360,45 @@ int transmit_flowfile(flow_file_record *ff, nifi_instance *instance) {
flow * create_new_flow(nifi_instance * instance) {
auto minifi_instance_ref = static_cast<minifi::Instance*>(instance->instance_ptr);
- flow *new_flow = (flow*) malloc(sizeof(flow));
-
- auto execution_plan = new ExecutionPlan(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
-
- new_flow->plan = execution_plan;
-
- return new_flow;
+ return new flow(minifi_instance_ref->getContentRepository(), minifi_instance_ref->getNoOpRepository(), minifi_instance_ref->getNoOpRepository());
--- End diff --
https://issues.apache.org/jira/browse/MINIFICPP-645 exists because public facing objects that are turned from a C API can be malloc'd by users. If a consumer of our API doesn't use free as designed, we should guarantee that they only cause memory leaks, not introduce undefined behavior.
---
[GitHub] nifi-minifi-cpp issue #432: MINIFICPP-648 - add processor and add processor ...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/432
> @arpadboda is this good? I'm good with this otherwise.
Good, but not complete, I will amend some stuff soon. (today)
---