Re: [PR] MINIFICPP-1415 Pass references to onTrigger and onSchedule instead of… [nifi-minifi-cpp]
fgerlits closed pull request #1693: MINIFICPP-1415 Pass references to onTrigger and onSchedule instead of… URL: https://github.com/apache/nifi-minifi-cpp/pull/1693 -- 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
Re: [PR] MINIFICPP-1415 Pass references to onTrigger and onSchedule instead of… [nifi-minifi-cpp]
szaszm commented on code in PR #1693: URL: https://github.com/apache/nifi-minifi-cpp/pull/1693#discussion_r1418972284 ## extensions/splunk/PutSplunkHTTP.cpp: ## Review Comment: In this case, I guess we can leave it as is for now. -- 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
Re: [PR] MINIFICPP-1415 Pass references to onTrigger and onSchedule instead of… [nifi-minifi-cpp]
martinzink commented on code in PR #1693: URL: https://github.com/apache/nifi-minifi-cpp/pull/1693#discussion_r1417112439 ## extensions/splunk/PutSplunkHTTP.cpp: ## Review Comment: I've checked we have a bunch of processors (mainly source processors, e.g. ConsumeWindowsEventLog, ListS3 etc...) that evaluate without flowfile context and they document their EL support the same way. So I am not sure if we want to manually overwrite the documentation. Maybe we could include this information inside the Property description? -- 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
Re: [PR] MINIFICPP-1415 Pass references to onTrigger and onSchedule instead of… [nifi-minifi-cpp]
martinzink commented on code in PR #1693: URL: https://github.com/apache/nifi-minifi-cpp/pull/1693#discussion_r1415648626 ## extensions/splunk/PutSplunkHTTP.cpp: ## Review Comment: I think most (if not all) of PROCESSORS.md is auto generated, so every attribute that has some level of expression langauge support via the `.supportsExpressionLanguage(true)` will be flagged as EL supported. Not sure if we want to deviate from the auto generated docs (since similar properties should be flagged as EL supported (due to auto generation). I think the best solution would be the introduce the concept of different levels of EL support, like NiFi does. -- 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
Re: [PR] MINIFICPP-1415 Pass references to onTrigger and onSchedule instead of… [nifi-minifi-cpp]
szaszm commented on code in PR #1693: URL: https://github.com/apache/nifi-minifi-cpp/pull/1693#discussion_r1410667988 ## extensions/splunk/PutSplunkHTTP.cpp: ## Review Comment: I suggest removing the "Supports expression language: true" part from PROCESSORS.md. I think similar properties that are evaluated without flow file context are also missing that line. -- 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
Re: [PR] MINIFICPP-1415 Pass references to onTrigger and onSchedule instead of… [nifi-minifi-cpp]
martinzink commented on code in PR #1693: URL: https://github.com/apache/nifi-minifi-cpp/pull/1693#discussion_r1395661548 ## extensions/splunk/PutSplunkHTTP.cpp: ## Review Comment: Yeah, the general expressions should still work, maybe we should be more explicit about how expressions are evaluated like nifi does. (but thats a more general problem) -- 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
Re: [PR] MINIFICPP-1415 Pass references to onTrigger and onSchedule instead of… [nifi-minifi-cpp]
martinzink commented on code in PR #1693: URL: https://github.com/apache/nifi-minifi-cpp/pull/1693#discussion_r1395661548 ## extensions/splunk/PutSplunkHTTP.cpp: ## Review Comment: Yeah, the general expressions should still work. -- 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
Re: [PR] MINIFICPP-1415 Pass references to onTrigger and onSchedule instead of… [nifi-minifi-cpp]
szaszm commented on code in PR #1693: URL: https://github.com/apache/nifi-minifi-cpp/pull/1693#discussion_r1395656597 ## extensions/splunk/PutSplunkHTTP.cpp: ## Review Comment: In that case, we should change the docs to indicate that EL is not supported for those properties. Or do we still evaluate expressions, just without the flow file context? -- 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
Re: [PR] MINIFICPP-1415 Pass references to onTrigger and onSchedule instead of… [nifi-minifi-cpp]
martinzink commented on code in PR #1693: URL: https://github.com/apache/nifi-minifi-cpp/pull/1693#discussion_r1395342750 ## extensions/http-curl/processors/InvokeHTTP.cpp: ## Review Comment: Same here > This was intentional, it was badly designed before. The creation of the client is only done when there is no available clients, and they are reused afterhand, so setting up a client based on a flowfile then using it with an other flowfile was incorrect. -- 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
Re: [PR] MINIFICPP-1415 Pass references to onTrigger and onSchedule instead of… [nifi-minifi-cpp]
martinzink commented on code in PR #1693: URL: https://github.com/apache/nifi-minifi-cpp/pull/1693#discussion_r1395342638 ## extensions/splunk/PutSplunkHTTP.cpp: ## Review Comment: This was intentional, it was badly designed before. The creation of the client is only done when there is no available clients, and they are reused afterhand, so setting up a client based on a flowfile then using it with an other flowfile was incorrect. ## extensions/http-curl/processors/InvokeHTTP.cpp: ## Review Comment: Same here -- 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
Re: [PR] MINIFICPP-1415 Pass references to onTrigger and onSchedule instead of… [nifi-minifi-cpp]
szaszm commented on code in PR #1693:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1693#discussion_r1394504470
##
extensions/http-curl/processors/InvokeHTTP.cpp:
##
Review Comment:
Same here: flow file attributes are no longer usable in the expression
language. While InvokeHTTP doesn't document any EL support, I'd rather not
break behavior, and possibly document the properties that actually support EL
with flow file context.
##
extensions/splunk/PutSplunkHTTP.cpp:
##
Review Comment:
The changes to this processor kill EL support: referencing flow file
attributes is no longer possible if we evaluate the changed properties in
`onSchedule`. I'd prefer to keep the old behavior.
##
extensions/standard-processors/processors/ListenTCP.h:
##
@@ -86,7 +86,7 @@ class ListenTCP : public NetworkListenerProcessor {
EXTENSIONAPI static constexpr auto OutputAttributes =
std::array{PortOutputAttribute, Sender};
void initialize() override;
- void onSchedule(const std::shared_ptr& context, const
std::shared_ptr& sessionFactory) override;
+ void onSchedule(core::ProcessContext& context, core::ProcessSessionFactory&
session_Factory) override;
Review Comment:
```suggestion
void onSchedule(core::ProcessContext& context,
core::ProcessSessionFactory& sessionFactory) override;
```
or
```suggestion
void onSchedule(core::ProcessContext& context,
core::ProcessSessionFactory& session_factory) override;
```
--
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
Re: [PR] MINIFICPP-1415 Pass references to onTrigger and onSchedule instead of… [nifi-minifi-cpp]
adamdebreceni commented on code in PR #1693:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1693#discussion_r1386727993
##
extensions/http-curl/processors/InvokeHTTP.cpp:
##
@@ -202,15 +186,15 @@ bool InvokeHTTP::appendHeaders(const core::FlowFile&
flow_file, /*std::invocable
return true;
}
-void InvokeHTTP::onTrigger(const std::shared_ptr&
context, const std::shared_ptr& session) {
- gsl_Expects(session && context && client_queue_);
+void InvokeHTTP::onTrigger(core::ProcessContext& context,
core::ProcessSession& session) {
+ gsl_Expects(client_queue_);
- auto flow_file = session->get();
+ auto flow_file = session.get();
if (flow_file == nullptr) {
if (!shouldEmitFlowFile()) {
- logger_->log_debug("InvokeHTTP -- create flow file with {}",
magic_enum::enum_name(method_));
- flow_file = session->create();
+ logger_->log_debug("InvokeHTTP -- create flow file with {}",
std::string(magic_enum::enum_name(method_)));
Review Comment:
why do we need to construct and `std::string`? `enum_name` seems to be used
without that in other logs
--
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
Re: [PR] MINIFICPP-1415 Pass references to onTrigger and onSchedule instead of… [nifi-minifi-cpp]
adamdebreceni commented on code in PR #1693:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1693#discussion_r1386514328
##
extensions/http-curl/processors/InvokeHTTP.cpp:
##
@@ -93,6 +64,9 @@ void
setupClientTransferEncoding(extensions::curl::HTTPClient& client, bool use_
} // namespace
void InvokeHTTP::setupMembersFromProperties(const core::ProcessContext&
context) {
+ if (!context.getProperty(URL, url_))
+throw Exception(PROCESS_SCHEDULE_EXCEPTION, "URL property missing or
invalid");
Review Comment:
oh I see it was moved, in that case it can stay as is to be easier to review
the PR
--
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
Re: [PR] MINIFICPP-1415 Pass references to onTrigger and onSchedule instead of… [nifi-minifi-cpp]
adamdebreceni commented on code in PR #1693:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1693#discussion_r1386506918
##
extensions/http-curl/processors/InvokeHTTP.cpp:
##
@@ -93,6 +64,9 @@ void
setupClientTransferEncoding(extensions::curl::HTTPClient& client, bool use_
} // namespace
void InvokeHTTP::setupMembersFromProperties(const core::ProcessContext&
context) {
+ if (!context.getProperty(URL, url_))
+throw Exception(PROCESS_SCHEDULE_EXCEPTION, "URL property missing or
invalid");
Review Comment:
can we use `getRequiredPropertyOrThrow` here?
--
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
Re: [PR] MINIFICPP-1415 Pass references to onTrigger and onSchedule instead of… [nifi-minifi-cpp]
martinzink commented on code in PR #1693:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1693#discussion_r1371429026
##
libminifi/include/core/Processor.h:
##
@@ -180,25 +180,24 @@ class Processor : public Connectable, public
ConfigurableComponent, public state
bool addConnection(Connectable* connection);
- virtual void onTrigger(const std::shared_ptr , const
std::shared_ptr );
- void onTrigger(ProcessContext *context, ProcessSessionFactory
*sessionFactory);
-
bool canEdit() override {
return !isRunning();
}
- virtual void onTrigger(const std::shared_ptr , const
std::shared_ptr ) {
-onTrigger(context.get(), session.get());
- }
- virtual void onTrigger(ProcessContext* /*context*/, ProcessSession*
/*session*/) {
- }
void initialize() override {
}
- virtual void onSchedule(const std::shared_ptr ,
const std::shared_ptr ) {
-onSchedule(context.get(), sessionFactory.get());
+
+ virtual void onTrigger(const std::shared_ptr& context, const
std::shared_ptr& session_factory);
+
+ virtual void onTrigger_2(const std::shared_ptr& context,
const std::shared_ptr& session) {
Review Comment:
I couldnt come up with a better name yet, I've changed it from onTrigger so
its easier to find the remaining occurences where we rely on shared_ptr-s
We could either revert this back to onTrigger or come up with a slightly
better name?
--
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
[PR] MINIFICPP-1415 Pass references to onTrigger and onSchedule instead of… [nifi-minifi-cpp]
martinzink opened a new pull request, #1693: URL: https://github.com/apache/nifi-minifi-cpp/pull/1693 … shared_ptr 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- where 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
