MINIFICPP-269 Implement ApplyTemplate processor by calebj · Pull Request #157 · apache/nifi-minifi-cpp

calebj · 2017-10-27T23:13:03Z

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 MINIFI-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?
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.

ai-christianson · 2017-10-28T21:38:59Z

The code looks very clean and straightforward. Great work.

I can definitely see the utility in such a processor for efficiently generating arbitrary textual formats from structured inputs.

I will build and test this and get back to you if I notice any issues.

ai-christianson · 2017-10-28T22:07:27Z

libminifi/test/bustache-tests/ApplyTemplateTests.cpp

+    std::shared_ptr<core::Processor> processor = std::make_shared<org::apache::nifi::minifi::processors::ApplyTemplate>("processorname");
+    REQUIRE(processor->getName() == "processorname");
+    uuid_t processoruuid;
+    REQUIRE(true == processor->getUUID(processoruuid));


Minor nit: simplify to REQUIRE(processor->getUUID(processoruuid));

ai-christianson · 2017-10-28T22:08:08Z

libminifi/test/bustache-tests/ApplyTemplateTests.cpp

+    std::stringstream output_buf;
+    output_buf << output_file.rdbuf();
+    std::string output_contents = output_buf.str();
+    REQUIRE(output_contents == EXPECT_OUTPUT);


Nice, concise test condition.

ai-christianson · 2017-10-28T22:14:57Z

There are a few clang-tidy issues that come up, which are mostly valid if minor.

Other than that, looks good to me.

phrocker · 2017-10-28T22:58:47Z

libminifi/CMakeLists.txt

 include_directories(../thirdparty/civetweb-1.9.1/include)
 include_directories(../thirdparty/jsoncpp/include)
 include_directories(../thirdparty/concurrentqueue/)
+include_directories(../thirdparty/bustache/include)


You shouldn't need to include this here, but rather in the your extension's CMakeLists

phrocker · 2017-10-28T22:59:05Z

extensions/bustache/ApplyTemplate.h

+    private:
+        //! Logger
+        std::shared_ptr<logging::Logger> logger_;
+        core::ProcessContext *_ctx;


Please use the google code style.

http://google.github.io/styleguide/cppguide.html#Variable_Names

phrocker · 2017-10-28T23:00:14Z

extensions/bustache/ApplyTemplate.h

+    private:
+        //! Logger
+        std::shared_ptr<logging::Logger> logger_;
+        core::ProcessContext *_ctx;


Avoid the use of storing the raw pointer. Functors exist to provide on trigger and onschedule with the shared pointers. Please use those instead.

phrocker · 2017-10-30T23:25:31Z

extensions/bustache/ApplyTemplate.h

+    ApplyTemplate(std::string name, uuid_t uuid = NULL)
+    : Processor(name, uuid)
+    {
+        logger_ = logging::LoggerFactory<ApplyTemplate>::getLogger();


With logger_ we should consider using the initializer list

phrocker · 2017-11-02T13:55:38Z

extensions/bustache/ApplyTemplate.cpp

+    setSupportedRelationships(relationships);
+}
+
+void ApplyTemplate::onTrigger(core::ProcessContext *context, core::ProcessSession *session) {


The shared ptr variants of these have for a while, so let's move to that for new processors. While the raw pointers exist I want to ensure new processors used the shared pointer calls.

phrocker · 2017-11-02T13:55:52Z

extensions/bustache/ApplyTemplate.cpp

+    flowFile_ = flowFile;
+}
+
+int64_t ApplyTemplate::WriteCallback::process(std::shared_ptr<io::BaseStream> stream) {


const ref on this call.

It won't let me declare a different prototype than the virtual one in OutputStreamCallback

[ 85%] Building CXX object extensions/bustache/CMakeFiles/minifi-bustache-extensions.dir/ApplyTemplate.cpp.o /home/ubuntu/workspace/extensions/bustache/ApplyTemplate.cpp: In member function ‘virtual void org::apache::nifi::minifi::processors::ApplyTemplate::onTrigger(const std::shared_ptr<org::apache::nifi::minifi::core::ProcessContext>&, const std::shared_ptr<org::apache::nifi::minifi::core::ProcessSession>&)’: /home/ubuntu/workspace/extensions/bustache/ApplyTemplate.cpp:66:19: error: cannot declare variable ‘cb’ to be of abstract type ‘org::apache::nifi::minifi::processors::ApplyTemplate::WriteCallback’ WriteCallback cb(templateFile, std::shared_ptr<core::FlowFile>(flowFile)); ^ In file included from /home/ubuntu/workspace/extensions/bustache/ApplyTemplate.cpp:32:0: /home/ubuntu/workspace/extensions/bustache/ApplyTemplate.h:61:11: note: because the following virtual functions are pure within ‘org::apache::nifi::minifi::processors::ApplyTemplate::WriteCallback’: class WriteCallback : public OutputStreamCallback { ^ In file included from /home/ubuntu/workspace/extensions/bustache/../../libminifi/include/core/ProcessSession.h:31:0, from /home/ubuntu/workspace/extensions/bustache/../../libminifi/include/core/Processor.h:42, from /home/ubuntu/workspace/extensions/bustache/ApplyTemplate.h:25, from /home/ubuntu/workspace/extensions/bustache/ApplyTemplate.cpp:32: /home/ubuntu/workspace/extensions/bustache/../../libminifi/include/FlowFileRecord.h:97:19: note: virtual int64_t org::apache::nifi::minifi::OutputStreamCallback::process(std::shared_ptr<org::apache::nifi::minifi::io::BaseStream>) virtual int64_t process(std::shared_ptr<io::BaseStream> stream) = 0; ^ make[2]: *** [extensions/bustache/CMakeFiles/minifi-bustache-extensions.dir/ApplyTemplate.cpp.o] Error 1 make[1]: *** [extensions/bustache/CMakeFiles/minifi-bustache-extensions.dir/all] Error 2 make: *** [all] Error 2

phrocker · 2017-11-02T13:58:11Z

extensions/bustache/ApplyTemplate.h

+    class WriteCallback : public OutputStreamCallback {
+    public:
+        WriteCallback(const std::string& templateFile, std::shared_ptr<core::FlowFile> flowFile);
+        int64_t process(std::shared_ptr<io::BaseStream> stream);


const ref shared pointers here, please.

phrocker · 2017-11-02T13:58:41Z

extensions/bustache/TemplateLoader.h

+  }
+
+  virtual std::unique_ptr<ObjectFactory> assign(const std::string &class_name) {
+    std::string name = class_name;


seems like dead code here.

calebj · 2017-11-13T18:56:37Z

Is there an estimate as to when this can be merged?

phrocker · 2017-11-14T00:48:23Z

CMakeLists.txt


+## Bustache/template extensions
+option(DISABLE_BUSTACHE "Disables Bustache (ApplyTemplate) support." OFF)
+if (NOT DISABLE_BUSTACHE)


Let's make this ENABLE_BUSTACHE

phrocker · 2017-11-28T17:20:46Z

CMakeLists.txt

 endif()

+## Bustache/template extensions
+option(ENABLE_BUSTACHE "Enables Bustache (ApplyTemplate) support." OFF)


These seem to conflict.

Your conditional is DISABLE_BUSTACHE. Your option is ENABLE_BUSTACHE. It seems to only be enabled if you DISABLE_BUSTACHE per the conditional below?

Ah, my bad. Fixing that uncovered a missing include, but both issues are resolved in the latest push.

Thanks. I'll merge shortly

edit: I'll wait for travis. I had a build failure on my mac:

/Library/Developer/CommandLineTools/usr/include/c++/v1/type_traits:4291:23: error: calling 'operator()' with incomplete return type 'bustache::value' _LIBCPP_INVOKE_RETURN(_VSTD::forward<_Fp>(__f)(_VSTD::forward<_Args>(__args)...)) ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /Library/Developer/CommandLineTools/usr/include/c++/v1/__config:392:15: note: expanded from macro '_VSTD' #define _VSTD std::_LIBCPP_NAMESPACE ^ /Library/Developer/CommandLineTools/usr/include/c++/v1/type_traits:4188:23: note: expanded from macro '_LIBCPP_INVOKE_RETURN' noexcept(noexcept(__VA_ARGS__)) -> decltype(__VA_ARGS__) \ ^~~~~~~~~~~ /Library/Developer/CommandLineTools/usr/include/c++/v1/type_traits:4308:9: note: in instantiation of exception specification for '__invoke<std::__1::function<bustache::value (const std::__1::vector<bustache::ast::content, std::__1::allocator<bustache::ast::content> > &)> &, const std::__1::vector<bustache::ast::content, std::__1::allocator<bustache::ast::content> > &>' requested here _VSTD::__invoke(_VSTD::declval<_Fp>(), _VSTD::declval<_Args>()...)); ^ /Library/Developer/CommandLineTools/usr/include/c++/v1/__config:392:15: note: expanded from macro '_VSTD' #define _VSTD std::_LIBCPP_NAMESPACE ^ /Library/Developer/CommandLineTools/usr/include/c++/v1/functional:1591:33: note: in instantiation of template class 'std::__1::__invokable_r<void, std::__1::function<bustache::value (const std::__1::vector<bustache::ast::content, std::__1::allocator<bustache::ast::content> > &)> &, const std::__1::vector<bustache::ast::content, std::__1::allocator<bustache::ast::content> > &>' requested here __invokable<_Fp&, _ArgTypes...>::value> ^ /Library/Developer/CommandLineTools/usr/include/c++/v1/functional:1616:9: note: in instantiation of default argument for '__callable<std::__1::function<bustache::value (const std::__1::vector<bustache::ast::content, std::__1::allocator<bustache::ast::content> > &)> >' required here __callable<_Fp>::value && !is_same<_Fp, function>::value ^~~~~~~~~~~~~~~ /Library/Developer/CommandLineTools/usr/include/c++/v1/functional:1618:5: note: in instantiation of default argument for 'function<std::__1::function<bustache::value (const std::__1::vector<bustache::ast::content, std::__1::allocator<bustache::ast::content> > &)> >' required here function(_Fp); ^~~~~~~~~~~~~ /Library/Developer/CommandLineTools/usr/include/c++/v1/functional:1578:28: note: while substituting deduced template arguments into function template 'function' [with _Fp = std::__1::function<bustache::value (const std::__1::vector<bustache::ast::content, std::__1::allocator<bustache::ast::content> > &)>, $1 = (no value)] class _LIBCPP_TEMPLATE_VIS function<_Rp(_ArgTypes...)> ^ /Library/Developer/CommandLineTools/usr/include/c++/v1/functional:1670:9: note: 'operator()' declared here _Rp operator()(_ArgTypes...) const; ^ /Users/mparisi/code/apache/nifi-minifi-cpp/thirdparty/bustache/include/bustache/model.hpp:56:11: note: definition of 'bustache::value' is not complete until the closing '}' class value : public variant_base<value> ^

phrocker · 2017-11-28T23:27:08Z

@calebj change your root env in travis to be

CMAKE_BUILD_OPTIONS="-DENABLE_PCAP=TRUE -DENABLE_BUSTACHE=TRUE"

Unless this is a linux only extension. Is it? If it is you'll have to include the previous CMAKE_BUILD_OPTIONS. As it is now linux is ignoring PCAP because you defined it in linux and overwrote the env option.

phrocker · 2017-11-29T15:23:55Z

@calebj I saw that your Travis imp run didn't build bustache, so I tried it myself. I added global to the root env https://github.com/phrocker/nifi-minifi-cpp/blob/ApplyTemplate/.travis.yml It seemed to run and build it.

I then noticed Travis passed while my build continues to Fail. Would you be okay adding A "Known Issues" referencing https://bugs.llvm.org/show_bug.cgi?id=34298 in regards to bustache not building on certain versions of os x? @apiri have any thoughts on this?

calebj · 2017-11-29T18:24:27Z

@phrocker Sure, I'll do that when I get a chance to.

Move ENABLE_BUSTACHE to travis root env This closes apache#157. Signed-off-by: Marc Parisi <phrocker@apache.org>

ai-christianson reviewed Oct 28, 2017

View reviewed changes

calebj changed the title ~~MINIFI-269 Implement ApplyTemplate processor~~ MINIFICPP-269 Implement ApplyTemplate processor Oct 30, 2017

calebj force-pushed the ApplyTemplate branch 2 times, most recently from 5600f29 to f1ee73a Compare October 30, 2017 11:31

phrocker requested changes Oct 30, 2017

View reviewed changes

calebj force-pushed the ApplyTemplate branch 6 times, most recently from 4989cec to 5f582dd Compare November 2, 2017 11:48

phrocker reviewed Nov 2, 2017

View reviewed changes

calebj force-pushed the ApplyTemplate branch 10 times, most recently from 8e10ed3 to f09be82 Compare November 9, 2017 15:07

calebj force-pushed the ApplyTemplate branch 2 times, most recently from 11cf1e8 to ed79c61 Compare November 13, 2017 17:03

phrocker reviewed Nov 14, 2017

View reviewed changes

calebj force-pushed the ApplyTemplate branch from ed79c61 to 98089ca Compare November 14, 2017 02:12

calebj force-pushed the ApplyTemplate branch 7 times, most recently from 4cc44a8 to 40f9237 Compare November 20, 2017 12:31

calebj force-pushed the ApplyTemplate branch 7 times, most recently from 54cbeb6 to b7fcca1 Compare November 27, 2017 19:31

calebj force-pushed the ApplyTemplate branch from b7fcca1 to ac18494 Compare November 28, 2017 14:24

phrocker reviewed Nov 28, 2017

View reviewed changes

calebj force-pushed the ApplyTemplate branch 2 times, most recently from 6c72d27 to acae2d3 Compare November 28, 2017 20:02

calebj force-pushed the ApplyTemplate branch from 4d514f6 to c9e4e90 Compare November 29, 2017 17:50

calebj force-pushed the ApplyTemplate branch from c9e4e90 to bfcff1d Compare December 5, 2017 12:10

calebj and others added 3 commits December 11, 2017 12:31

MINIFICPP-269 Implement ApplyTemplate processor

b643d98

Move ENABLE_BUSTACHE to travis root env

542d025

ApplyTemplate

0892bb5

calebj force-pushed the ApplyTemplate branch from bfcff1d to 0892bb5 Compare December 11, 2017 13:42

asfgit closed this in d4f0bcf Dec 13, 2017

calebj deleted the ApplyTemplate branch December 13, 2017 22:23

nghiaxlee pushed a commit to nghiaxlee/nifi-minifi-cpp that referenced this pull request Jul 8, 2019

MINIFICPP-269 Implement ApplyTemplate processor

d6bd0ca

Move ENABLE_BUSTACHE to travis root env This closes apache#157. Signed-off-by: Marc Parisi <phrocker@apache.org>

Comments

Conversation

calebj commented Oct 27, 2017

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

ai-christianson commented Oct 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ai-christianson commented Oct 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calebj commented Nov 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phrocker Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phrocker Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phrocker commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phrocker commented Nov 29, 2017

Uh oh!

calebj commented Nov 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

phrocker Nov 28, 2017 •

edited

Loading

phrocker Nov 28, 2017 •

edited

Loading

phrocker commented Nov 28, 2017 •

edited

Loading