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/29 13:29:16 UTC
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
GitHub user arpadboda opened a pull request:
https://github.com/apache/nifi-minifi-cpp/pull/429
MINIFICPP-654 - C API: failure callback improvements
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-654
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi-minifi-cpp/pull/429.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 #429
----
commit e229d2abe141f8e84b010f3d6500a3be894f20ac
Author: Arpad Boda <ab...@...>
Date: 2018-10-24T13:14:05Z
MINIFICPP-654 - C API: failure callback improvements
----
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
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/429#discussion_r229265673
--- Diff: libminifi/include/capi/Plan.h ---
@@ -16,6 +16,8 @@
* limitations under the License.
*/
+#pragma once
--- End diff --
We should keep to the standards.
---
[GitHub] nifi-minifi-cpp issue #429: MINIFICPP-654 - C API: failure callback improvem...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/429
Looks good. minor comments, nothing that's a big deal
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
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/429#discussion_r229018550
--- Diff: libminifi/src/capi/api.cpp ---
@@ -170,31 +165,31 @@ flow_file_record* create_ff_object_na(const char *file, const size_t len, const
new_ff->attributes = nullptr;
new_ff->contentLocation = new char[len + 1];
snprintf(new_ff->contentLocation, len + 1, "%s", file);
- std::ifstream in(file, std::ifstream::ate | std::ifstream::binary);
// set the size of the flow file.
new_ff->size = size;
+ new_ff->crp = static_cast<void*>(new std::shared_ptr<minifi::core::ContentRepository>);
--- End diff --
make_shared ?
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
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/429#discussion_r229107909
--- Diff: libminifi/include/capi/Plan.h ---
@@ -46,11 +48,50 @@
#include "capi/cstructs.h"
#include "capi/api.h"
+enum FailureStrategy { AS_IS, ROLLBACK };
+
+bool intToFailureStragey(int in, FailureStrategy *out);
+
+using callback_type = std::function<void(flow_file_record*)>;
--- End diff --
Sure, I will rename this, should be failure callback type or something alike.
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
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/429#discussion_r229015052
--- Diff: libminifi/include/capi/Plan.h ---
@@ -16,6 +16,8 @@
* limitations under the License.
*/
+#pragma once
--- End diff --
The more I think about this for a public, open source project, we should probably avoid pragma once given that it's implementation defined and not language spec. We don't control all compiler variants, especially for what will be this portion of the API. I realize it's nearly "universally supported" but it seems like an unnecessary risk, albeit small.
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
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/429#discussion_r229263303
--- Diff: libminifi/include/capi/Plan.h ---
@@ -46,11 +48,50 @@
#include "capi/cstructs.h"
#include "capi/api.h"
+enum FailureStrategy { AS_IS, ROLLBACK };
+
+bool intToFailureStragey(int in, FailureStrategy *out);
+
+using callback_type = std::function<void(flow_file_record*)>;
+using content_repo_sptr = std::shared_ptr<core::ContentRepository>;
+
+namespace {
+
+ void failureStrategyAsIs(core::ProcessSession *session, callback_type user_callback, content_repo_sptr cr_ptr) {
+ auto ff = session->get();
+ if (ff == nullptr) {
+ return;
+ }
+
+ auto claim = ff->getResourceClaim();
+
+ if (claim != nullptr && user_callback != nullptr) {
+ claim->increaseFlowFileRecordOwnedCount();
+ // create a flow file.
+ auto path = claim->getContentFullPath();
+ auto ffr = create_ff_object_na(path.c_str(), path.length(), ff->getSize());
+ ffr->attributes = ff->getAttributesPtr();
+ ffr->ffp = ff.get();
+ auto content_repo_ptr = static_cast<std::shared_ptr<minifi::core::ContentRepository>*>(ffr->crp);
+ *content_repo_ptr = cr_ptr;
+ user_callback(ffr);
+ }
+ session->remove(ff);
+ }
+
+ void failureStrategyRollback(core::ProcessSession *session, callback_type user_callback, content_repo_sptr cr_ptr) {
+ session->rollback();
+ failureStrategyAsIs(session, user_callback, cr_ptr);
+ }
+}
+
+static const std::map<FailureStrategy, const std::function<void(core::ProcessSession*, callback_type, content_repo_sptr)>> FailureStrategies =
--- End diff --
For extendability and readability reasons I would prefer to keep the map. As it's failure handling, I don't think the minor overhead introduced by the map matters.
---
[GitHub] nifi-minifi-cpp issue #429: MINIFICPP-654 - C API: failure callback improvem...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/429
@arpadboda is this ready for review? You stated, "it's not final"
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
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/429#discussion_r229107701
--- Diff: libminifi/include/capi/Plan.h ---
@@ -46,11 +48,50 @@
#include "capi/cstructs.h"
#include "capi/api.h"
+enum FailureStrategy { AS_IS, ROLLBACK };
--- End diff --
I agree with you that it's not the best naming as doesn't properly describe what happens, but no better idea. @apiri - you are our last hope. :)
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
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/429#discussion_r229266790
--- Diff: libminifi/include/capi/Plan.h ---
@@ -16,6 +16,8 @@
* limitations under the License.
*/
+#pragma once
--- End diff --
Removed.
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
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/429#discussion_r229020814
--- Diff: libminifi/src/capi/api.cpp ---
@@ -400,11 +395,19 @@ processor *add_processor_with_linkage(flow *flow, const char *processor_name) {
return nullptr;
}
-int add_failure_callback(flow *flow, void (*onerror_callback)(const flow_file_record*)) {
+int add_failure_callback(flow *flow, void (*onerror_callback)(flow_file_record*)) {
ExecutionPlan *plan = static_cast<ExecutionPlan*>(flow->plan);
return plan->setFailureCallback(onerror_callback) ? 0 : 1;
}
+int set_failure_strategy(flow *flow, int strategy) {
--- End diff --
Why is strategy not typed to an enumeration? Might change the dynamics of the return value but it might make conversions more safe.
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
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/429#discussion_r229015497
--- Diff: libminifi/include/capi/Plan.h ---
@@ -46,11 +48,50 @@
#include "capi/cstructs.h"
#include "capi/api.h"
+enum FailureStrategy { AS_IS, ROLLBACK };
+
+bool intToFailureStragey(int in, FailureStrategy *out);
--- End diff --
*spelling.
Is this something to be used in C? Perhaps we should avoid booleans? stdbool wasn't introduced until C99..
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
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/429#discussion_r229264358
--- Diff: libminifi/include/capi/Plan.h ---
@@ -16,6 +16,8 @@
* limitations under the License.
*/
+#pragma once
--- End diff --
As it's Plan.h, this will surely be compiled with C++ compilers, which properly support according to my experience. I don't plan to add it to C API.
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
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/429#discussion_r229108406
--- Diff: libminifi/src/capi/api.cpp ---
@@ -170,31 +165,31 @@ flow_file_record* create_ff_object_na(const char *file, const size_t len, const
new_ff->attributes = nullptr;
new_ff->contentLocation = new char[len + 1];
snprintf(new_ff->contentLocation, len + 1, "%s", file);
- std::ifstream in(file, std::ifstream::ate | std::ifstream::binary);
// set the size of the flow file.
new_ff->size = size;
+ new_ff->crp = static_cast<void*>(new std::shared_ptr<minifi::core::ContentRepository>);
--- End diff --
As there are no arguments provided, I couldn't save specifying the template parameter. Moreover it's an empty shared ptr, so it doesn't even allocate twice.
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
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/429#discussion_r229016579
--- Diff: libminifi/include/capi/Plan.h ---
@@ -46,11 +48,50 @@
#include "capi/cstructs.h"
#include "capi/api.h"
+enum FailureStrategy { AS_IS, ROLLBACK };
--- End diff --
Do you think AS_IS could be clarified in the scope of the enumeration to better indicate the intent?
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi-minifi-cpp/pull/429
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
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/429#discussion_r229107427
--- Diff: libminifi/include/capi/Plan.h ---
@@ -46,11 +48,50 @@
#include "capi/cstructs.h"
#include "capi/api.h"
+enum FailureStrategy { AS_IS, ROLLBACK };
+
+bool intToFailureStragey(int in, FailureStrategy *out);
--- End diff --
Nope, it's hidden behind the C API, the API wraps it to int return value.
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
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/429#discussion_r229108728
--- Diff: libminifi/src/capi/api.cpp ---
@@ -400,11 +395,19 @@ processor *add_processor_with_linkage(flow *flow, const char *processor_name) {
return nullptr;
}
-int add_failure_callback(flow *flow, void (*onerror_callback)(const flow_file_record*)) {
+int add_failure_callback(flow *flow, void (*onerror_callback)(flow_file_record*)) {
ExecutionPlan *plan = static_cast<ExecutionPlan*>(flow->plan);
return plan->setFailureCallback(onerror_callback) ? 0 : 1;
}
+int set_failure_strategy(flow *flow, int strategy) {
--- End diff --
The reason is pretty simple: I didn't have enough coffee when I wrote this, so forgot that C also supports enums. :) Will replace.
---
[GitHub] nifi-minifi-cpp issue #429: MINIFICPP-654 - C API: failure callback improvem...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/429
It's not final, further changes expected as I'm not yet satisfied with the code quality, but the concept can be reviewed at least.
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
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/429#discussion_r229018124
--- Diff: libminifi/include/capi/Plan.h ---
@@ -46,11 +48,50 @@
#include "capi/cstructs.h"
#include "capi/api.h"
+enum FailureStrategy { AS_IS, ROLLBACK };
+
+bool intToFailureStragey(int in, FailureStrategy *out);
+
+using callback_type = std::function<void(flow_file_record*)>;
+using content_repo_sptr = std::shared_ptr<core::ContentRepository>;
+
+namespace {
+
+ void failureStrategyAsIs(core::ProcessSession *session, callback_type user_callback, content_repo_sptr cr_ptr) {
+ auto ff = session->get();
+ if (ff == nullptr) {
+ return;
+ }
+
+ auto claim = ff->getResourceClaim();
+
+ if (claim != nullptr && user_callback != nullptr) {
+ claim->increaseFlowFileRecordOwnedCount();
+ // create a flow file.
+ auto path = claim->getContentFullPath();
+ auto ffr = create_ff_object_na(path.c_str(), path.length(), ff->getSize());
+ ffr->attributes = ff->getAttributesPtr();
+ ffr->ffp = ff.get();
+ auto content_repo_ptr = static_cast<std::shared_ptr<minifi::core::ContentRepository>*>(ffr->crp);
+ *content_repo_ptr = cr_ptr;
+ user_callback(ffr);
+ }
+ session->remove(ff);
+ }
+
+ void failureStrategyRollback(core::ProcessSession *session, callback_type user_callback, content_repo_sptr cr_ptr) {
+ session->rollback();
+ failureStrategyAsIs(session, user_callback, cr_ptr);
+ }
+}
+
+static const std::map<FailureStrategy, const std::function<void(core::ProcessSession*, callback_type, content_repo_sptr)>> FailureStrategies =
--- End diff --
If there are only two is an ordered map or any map at all necessary? Would a simple switch suffice?
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
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/429#discussion_r229017394
--- Diff: libminifi/include/capi/Plan.h ---
@@ -46,11 +48,50 @@
#include "capi/cstructs.h"
#include "capi/api.h"
+enum FailureStrategy { AS_IS, ROLLBACK };
+
+bool intToFailureStragey(int in, FailureStrategy *out);
+
+using callback_type = std::function<void(flow_file_record*)>;
--- End diff --
^I'm not inherently against aliasing but I think 'callback_type' does little to improve readability, and since readability is in the eye of the beholder ( and I don't have a preference if others use it ) is there a way to at least better indicate that this is a function used for a certain type of callback?
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
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/429#discussion_r229021535
--- Diff: libminifi/include/capi/Plan.h ---
@@ -46,11 +48,50 @@
#include "capi/cstructs.h"
#include "capi/api.h"
+enum FailureStrategy { AS_IS, ROLLBACK };
+
+bool intToFailureStragey(int in, FailureStrategy *out);
+
+using callback_type = std::function<void(flow_file_record*)>;
+using content_repo_sptr = std::shared_ptr<core::ContentRepository>;
+
+namespace {
+
+ void failureStrategyAsIs(core::ProcessSession *session, callback_type user_callback, content_repo_sptr cr_ptr) {
+ auto ff = session->get();
+ if (ff == nullptr) {
+ return;
+ }
+
+ auto claim = ff->getResourceClaim();
+
+ if (claim != nullptr && user_callback != nullptr) {
+ claim->increaseFlowFileRecordOwnedCount();
+ // create a flow file.
+ auto path = claim->getContentFullPath();
+ auto ffr = create_ff_object_na(path.c_str(), path.length(), ff->getSize());
+ ffr->attributes = ff->getAttributesPtr();
+ ffr->ffp = ff.get();
+ auto content_repo_ptr = static_cast<std::shared_ptr<minifi::core::ContentRepository>*>(ffr->crp);
+ *content_repo_ptr = cr_ptr;
+ user_callback(ffr);
+ }
+ session->remove(ff);
+ }
+
+ void failureStrategyRollback(core::ProcessSession *session, callback_type user_callback, content_repo_sptr cr_ptr) {
+ session->rollback();
+ failureStrategyAsIs(session, user_callback, cr_ptr);
+ }
+}
+
+static const std::map<FailureStrategy, const std::function<void(core::ProcessSession*, callback_type, content_repo_sptr)>> FailureStrategies =
+ { { FailureStrategy::AS_IS, failureStrategyAsIs }, {FailureStrategy::ROLLBACK, failureStrategyRollback } };
class ExecutionPlan {
public:
- explicit ExecutionPlan(std::shared_ptr<core::ContentRepository> content_repo, std::shared_ptr<core::Repository> flow_repo, std::shared_ptr<core::Repository> prov_repo);
+ explicit ExecutionPlan(content_repo_sptr content_repo, std::shared_ptr<core::Repository> flow_repo, std::shared_ptr<core::Repository> prov_repo);
--- End diff --
^ Seems harder to read. Not sure if it helps but we have our linter set to 200 line characters ( to match Apache NiFi ), so we're not that tight on line space.
---
[GitHub] nifi-minifi-cpp pull request #429: MINIFICPP-654 - C API: failure callback i...
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/429#discussion_r229263761
--- Diff: libminifi/include/capi/Plan.h ---
@@ -46,11 +48,50 @@
#include "capi/cstructs.h"
#include "capi/api.h"
+enum FailureStrategy { AS_IS, ROLLBACK };
+
+bool intToFailureStragey(int in, FailureStrategy *out);
+
+using callback_type = std::function<void(flow_file_record*)>;
+using content_repo_sptr = std::shared_ptr<core::ContentRepository>;
+
+namespace {
+
+ void failureStrategyAsIs(core::ProcessSession *session, callback_type user_callback, content_repo_sptr cr_ptr) {
+ auto ff = session->get();
+ if (ff == nullptr) {
+ return;
+ }
+
+ auto claim = ff->getResourceClaim();
+
+ if (claim != nullptr && user_callback != nullptr) {
+ claim->increaseFlowFileRecordOwnedCount();
+ // create a flow file.
+ auto path = claim->getContentFullPath();
+ auto ffr = create_ff_object_na(path.c_str(), path.length(), ff->getSize());
+ ffr->attributes = ff->getAttributesPtr();
+ ffr->ffp = ff.get();
+ auto content_repo_ptr = static_cast<std::shared_ptr<minifi::core::ContentRepository>*>(ffr->crp);
+ *content_repo_ptr = cr_ptr;
+ user_callback(ffr);
+ }
+ session->remove(ff);
+ }
+
+ void failureStrategyRollback(core::ProcessSession *session, callback_type user_callback, content_repo_sptr cr_ptr) {
+ session->rollback();
+ failureStrategyAsIs(session, user_callback, cr_ptr);
+ }
+}
+
+static const std::map<FailureStrategy, const std::function<void(core::ProcessSession*, callback_type, content_repo_sptr)>> FailureStrategies =
+ { { FailureStrategy::AS_IS, failureStrategyAsIs }, {FailureStrategy::ROLLBACK, failureStrategyRollback } };
class ExecutionPlan {
public:
- explicit ExecutionPlan(std::shared_ptr<core::ContentRepository> content_repo, std::shared_ptr<core::Repository> flow_repo, std::shared_ptr<core::Repository> prov_repo);
+ explicit ExecutionPlan(content_repo_sptr content_repo, std::shared_ptr<core::Repository> flow_repo, std::shared_ptr<core::Repository> prov_repo);
--- End diff --
Agreed, it doesn't really help here.
---