MINIFI-236: Make GetFile, PutFile, TailFile, and ExecuteProcess ThreadSafe by phrocker · Pull Request #71 · apache/nifi-minifi-cpp

phrocker · 2017-03-30T00:18:31Z

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.

…d safe

apiri · 2017-03-30T12:08:10Z

reviewing

phrocker

Left some requests for changes.

phrocker · 2017-03-30T00:35:24Z

libminifi/src/core/ProcessSession.cpp

  }
 }

+void ProcessSession::importFrom(io::DataStream &stream,


Add comment.

phrocker · 2017-03-30T13:25:17Z

CMakeLists.txt

    target_include_directories(tests PRIVATE BEFORE "libminifi/include/provenance")
    target_link_libraries(tests ${CMAKE_THREAD_LIBS_INIT} ${UUID_LIBRARIES} ${LEVELDB_LIBRARIES} ${OPENSSL_LIBRARIES} minifi yaml-cpp c-library civetweb-cpp)
    add_test(NAME LibMinifiTests COMMAND tests)
+


Can we try and make this less duplicative?

phrocker · 2017-03-30T13:25:52Z

libminifi/include/core/ProcessSession.h

  void penalize(std::shared_ptr<core::FlowFile> &flow);
  void penalize(std::shared_ptr<core::FlowFile> &&flow);
 // Import the existed file into the flow
+  void importFrom(io::DataStream &stream, std::shared_ptr<core::FlowFile> &&flow);


better comment?

phrocker · 2017-03-30T13:26:32Z

libminifi/include/processors/GetFile.h

  // perform directory listing
-  void performListing(std::string dir);
+  void performListing(std::string dir,const GetFileRequest &request);
+


phrocker · 2017-03-30T13:27:17Z

libminifi/include/utils/ThreadPool.h

+   std::shared_ptr<std::promise<T>> promise;
+};
+
+template< typename T>


comment to remind people that && will take ownership of Worker

phrocker · 2017-03-30T13:29:12Z

libminifi/src/core/ProcessSession.cpp

+                            std::shared_ptr<core::FlowFile> &&flow) {
+  std::shared_ptr<ResourceClaim> claim = std::make_shared<ResourceClaim>();
+
+  int max_read = getpagesize();


is this portable?

phrocker · 2017-03-30T13:31:38Z

libminifi/src/processors/TailFile.cpp

  return (i.modifiedTime < j.modifiedTime);
 }
-void TailFile::checkRollOver() {
+void TailFile::checkRollOver(std::string fileLocation, std::string fileName) {


const arguments?

phrocker · 2017-03-30T13:32:10Z

libminifi/test/TestExecuteProcess.cpp

+  std::shared_ptr<core::ProcessContext> contextset = std::make_shared<
+      core::ProcessContext>(node2, test_repo);
+  core::ProcessSessionFactory factory(contextset.get());
+ // processor->onSchedule(contextset.get(), &factory);


unless this should be removed.

apiri

Overall things look good. I had made a few comments concerning formatting at first but preferred to just punt that to an effort for MINIFI-254. Only question I had was concerning the map usage in GetFile. Otherwise, looks good here.

apiri · 2017-03-30T12:20:51Z

libminifi/include/core/FlowConfiguration.h

 #include "processors/PutFile.h"
 #include "processors/TailFile.h"
 #include "processors/ListenSyslog.h"
+#include "processors/ListenHTTP.h"


minor: can we revert this shuffling?

apiri · 2017-03-30T12:21:17Z

libminifi/include/core/ConfigurableComponent.h

 * Purpose: Extracts configuration items for all components and localized them
 */
-class ConfigurableComponent {
+class ConfigurableComponent  {


minor: extraneous whitespace

apiri · 2017-03-30T14:56:35Z

libminifi/src/core/FlowConfiguration.cpp

      == org::apache::nifi::minifi::processors::LogAttribute::ProcessorName) {
    processor = std::make_shared<
        org::apache::nifi::minifi::processors::LogAttribute>(name, uuid);
+  } else if (name


minor: can we revert this shuffling?

apiri · 2017-03-30T16:19:39Z

libminifi/include/processors/GetFile.h

-  uint64_t _lastDirectoryListingTime;
-  std::string _fileFilter;
+
+  std::map<std::string,std::atomic<uint64_t>> last_listing_times_;


Not sure I follow the usage of the map. Presumably we only have one directory per instance as there is no input.

apiri · 2017-04-05T08:15:52Z

@phrocker Just wanted to confirm this was done before a final review. I think it likely makes sense to get this one in before we merge #73

apiri · 2017-04-05T14:33:14Z

reviewed the latest commit and code looks good

Verified builds in OS X 10.11, Centos 7.x, and OS X 10.12 (via #75)

Will get this merged and thank you for the fixes.

…d safe This closes apache#71. Signed-off-by: Aldrin Piri <aldrin@apache.org>

phrocker force-pushed the MINIFI-236-b branch 4 times, most recently from c2bb402 to e933e39 Compare March 30, 2017 00:34

MINIFI-236: Make GetFile, PutFile, TailFile, and ExecuteProcess threa…

ae72417

…d safe

phrocker force-pushed the MINIFI-236-b branch from e933e39 to ae72417 Compare March 30, 2017 00:37

phrocker commented Mar 30, 2017

View reviewed changes

apiri reviewed Mar 30, 2017

View reviewed changes

MINIFI-236: Updates per pull request

03759a4

phrocker force-pushed the MINIFI-236-b branch from 3fdc58e to 03759a4 Compare April 3, 2017 14:23

asfgit closed this in 3676468 Apr 5, 2017

apiri mentioned this pull request Apr 5, 2017

MINIFI-254: Incremental update for linter changes #73

Closed

8 tasks

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

MINIFI-236: Make GetFile, PutFile, TailFile, and ExecuteProcess threa…

f803788

…d safe This closes apache#71. Signed-off-by: Aldrin Piri <aldrin@apache.org>

Conversation

phrocker commented Mar 30, 2017

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

apiri commented Mar 30, 2017

Uh oh!

phrocker left a comment

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!

Choose a reason for hiding this comment

Uh oh!

apiri left a comment

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!

apiri commented Apr 5, 2017

Uh oh!

apiri commented Apr 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants