MINIFICPP-72: Add Tar and Zip Support for MergeContent #146

minifirocks · 2017-10-09T16:05:46Z

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.

phrocker · 2017-10-09T16:11:56Z

libminifi/include/processors/MergeContent.h

+      int64_t ret = 0;
+      uint64_t read_size;
+      ret = stream->read(buffer, buffer_size_);
+      if (!stream)


Checking whether stream is null after you attempt a function call on it seems counterintuitive.

!stream means that we read EOF after the operation, in this case, we did not get the full size.

phrocker · 2017-10-09T16:13:18Z

libminifi/include/processors/MergeContent.h

+            }
+          }
+        }
+        ReadCallback readCb(flow->getSize(), arch, entry);


We instantiate the entire buffer of the archive into memory? If that is our process, should we not have memory limits?

will try to do incremental read.

phrocker · 2017-10-09T16:13:49Z

extensions/http-curl/CMakeLists.txt

@@ -24,7 +24,7 @@ find_package(CURL REQUIRED)
 set(CMAKE_EXE_LINKER_FLAGS "-Wl,--export-all-symbols")
 set(CMAKE_SHARED_LINKER_FLAGS "-Wl,--export-symbols")

-include_directories(../../libminifi/include ../../libminifi/include/c2  ../../libminifi/include/c2/protocols/  ../../libminifi/include/core/state ./libminifi/include/core/statemanagement/metrics  ../../libminifi/include/core/yaml  ../../libminifi/include/core  ../../thirdparty/spdlog-20170710/include ../../thirdparty/concurrentqueue ../../thirdparty/yaml-cpp-yaml-cpp-0.5.3/include ../../thirdparty/civetweb-1.9.1/include ../../thirdparty/jsoncpp/include ../../thirdparty/leveldb-1.18/include ../../thirdparty/)
+include_directories(../../libminifi/include ../../libminifi/include/c2  ../../libminifi/include/c2/protocols/  ../../libminifi/include/core/state ./libminifi/include/core/statemanagement/metrics  ../../libminifi/include/core/yaml  ../../libminifi/include/core  ../../thirdparty/spdlog-20170710/include ../../thirdparty/concurrentqueue ../../thirdparty/yaml-cpp-yaml-cpp-0.5.3/include ../../thirdparty/civetweb-1.9.1/include ../../thirdparty/jsoncpp/include ../../thirdparty/leveldb-1.18/include ../../thirdparty/libarchive-3.3.2/libarchive ../../thirdparty/)


Do you recall the error that necessitated adding lib archive here?

Scanning dependencies of target minifi-http-curl
[ 15%] Building CXX object extensions/http-curl/CMakeFiles/minifi-http-curl.dir/HttpCurlLoader.cpp.o
In file included from /Users/binqiu/m/nifi-minifi-cpp/extensions/http-curl/HttpCurlLoader.cpp:20:
In file included from /Users/binqiu/m/nifi-minifi-cpp/extensions/http-curl/../../libminifi/include/core/FlowConfiguration.h:37:
/Users/binqiu/m/nifi-minifi-cpp/extensions/http-curl/../../libminifi/include/processors/MergeContent.h:24:10: fatal error: 'archive_entry.h' file not found

oh that's far too generic of an include directories statement, that's why. we shouldn't be cross compiling merge content for curl.

it is FlowConfiguration.h which include MergeContent.h

phrocker · 2017-10-09T16:14:50Z

libminifi/include/processors/MergeContent.h

+            try {
+              permInt = std::stoi(perm);
+              archive_entry_set_perm(entry, (mode_t) permInt);
+            } catch (...) {


Could we log perm when this happens?

@apiri @phrocker fixed.

phrocker · 2017-10-09T16:15:33Z

libminifi/src/processors/MergeContent.cpp

+    flows.front()->getAttribute(BinFiles::SEGMENT_ORIGINAL_FILENAME, fileName);
+  }
+  if (!fileName.empty()) {
+    fileName += ".tar";


what if your file is already a tar? Will we handle that case and then also rename it fileName.tar.tar?

same as blew zip. we will tar the tar file into a tar ball.

phrocker · 2017-10-09T16:16:27Z

libminifi/src/processors/MergeContent.cpp

+    flows.front()->getAttribute(BinFiles::SEGMENT_ORIGINAL_FILENAME, fileName);
+  }
+  if (!fileName.empty()) {
+    fileName += ".zip";


Same here...I don't really mind if it's named file.zip.zip, but if we have the flow going through zip twice, will we zip the two separate zips into a larger one or combine them? This isn't clear to me from reading the code.

yes, we will zip the two separate zip into a larger one

if two sep zip files fail into the same bin, we will create a single zip file which zip these two zip files.

very cool, thanks!

minifirocks · 2017-10-10T14:59:52Z

@phrocker please review the same
@apiri @jdye64 please review the LICENSE file for libarchive to see whether it is OK. Joe raise some concern in last PR.

minifirocks · 2017-10-11T22:35:56Z

@phrocker @apiri please let me know if you have comments.

apiri

@minifirocks I think the LICENSE needs to be revisited and left some thoughts on that. Also, were you able to adjust the incremental approach to avoid the allocation for memory as pointed out by Marc? Functionality looks okay otherwise and did some simple testing.

apiri · 2017-10-12T15:10:03Z

LICENSE

@@ -564,4 +564,55 @@ ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
 CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 SOFTWARE.
 ========================================================================
+The libarchive distribution as a whole is Copyright by Tim Kientzle


We need to be more detailed and specific in our LICENSE than this. We will need to break out particular items and their terms for those efforts as there are several licenses and considerations at play for the various components of this library.

I would additionally recommend removing all extraneous parts of libarchive to reduce this footprint such as examples and test resources and those build scripts unneeded for our build.

minifirocks · 2017-10-12T22:49:09Z

The incremental read already been added to address Marc comments.

minifirocks · 2017-10-13T00:40:00Z

@apiri remove test and doc from lib archieve, update LICENSE to add each author.

apiri

Need to do a bit more with the LICENSE terms as it is not currently in the correct format nor explicitly enumerating the various components of the library. I also noticed there are still a couple items that @phrocker had requested changes on you were going to evaluate that have not had any updates I can see around them (the incremental read of the archive and logging requests are the two that come to mind).

apiri · 2017-10-16T13:23:22Z

LICENSE

 (END LICENSE TEXT)
+
+Each individual file in this distribution should have a clear


We need to call out the specific items and not just include the boilerplate message by the source LICENSE.

In the case of those exclusions we must note that "the project includes <project/source> which is available under " and the associated copyright. As a reference, scope out the other items in this LICENSE to see how they are handled.

We should not have the caveats listed by the source but interpret them and include the appropriate clauses for those items which we do include.

apiri · 2017-10-16T13:24:06Z

LICENSE

@@ -564,4 +564,64 @@ ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
 CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 SOFTWARE.
 ========================================================================
+The libarchive distribution as a whole is Copyright by Tim Kientzle and others


This should be converted to a "This project includes ... which is available under ..." phrasing in lieu of taking the associated words from the contributing license.

minifirocks · 2017-10-16T16:42:15Z

LICENSE

@@ -564,4 +564,68 @@ ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
 CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE


@apiri fix the license

minifirocks · 2017-10-16T16:42:48Z

libminifi/include/processors/MergeContent.h

@@ -67,15 +70,19 @@ class BinaryConcatenationMerge : public MergeBin {
    ~ReadCallback() {
    }
    int64_t process(std::shared_ptr<io::BaseStream> stream) {
-      uint8_t buffer[buffer_size_];


@apiri fixed for incremental read

minifirocks · 2017-10-18T00:11:43Z

@apiri @phrocker rebased.

phrocker

+1 will merge as soon as I have a chance. Thanks!

This closes #146. Signed-off-by: Marc Parisi <phrocker@apache.org>

This closes apache#146. Signed-off-by: Marc Parisi <phrocker@apache.org>

phrocker requested changes Oct 9, 2017

View reviewed changes

minifirocks force-pushed the archive_merge branch from ed7989d to cd30d98 Compare October 10, 2017 14:57

apiri reviewed Oct 12, 2017

View reviewed changes

minifirocks force-pushed the archive_merge branch from cd30d98 to a902d3c Compare October 13, 2017 00:36

minifirocks changed the title ~~Archive merge~~ MINIFICPP-72: Add Tar and Zip Support for MergeContent Oct 13, 2017

minifirocks force-pushed the archive_merge branch from a902d3c to 117a63f Compare October 13, 2017 02:26

apiri requested changes Oct 16, 2017

View reviewed changes

minifirocks force-pushed the archive_merge branch from 117a63f to df1db1f Compare October 16, 2017 15:55

minifirocks commented Oct 16, 2017

View reviewed changes

minifirocks force-pushed the archive_merge branch from df1db1f to dc8bd76 Compare October 17, 2017 22:52

calebj mentioned this pull request Oct 19, 2017

MINIFI-244 Un/FocusArchive processors #148

Closed

8 tasks

minifirocks force-pushed the archive_merge branch from dc8bd76 to e94e9e2 Compare October 19, 2017 15:28

MINIFICPP-72: Add Tar and Zip Support for MergeContent

a13e520

minifirocks force-pushed the archive_merge branch from e94e9e2 to a13e520 Compare October 19, 2017 16:43

phrocker approved these changes Oct 20, 2017

View reviewed changes

minifirocks closed this Oct 20, 2017

minifirocks deleted the archive_merge branch October 20, 2017 20:11

asfgit pushed a commit that referenced this pull request Oct 20, 2017

MINIFICPP-72: Add Tar and Zip Support for MergeContent

3781b52

This closes #146. Signed-off-by: Marc Parisi <phrocker@apache.org>

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

MINIFICPP-72: Add Tar and Zip Support for MergeContent

87c7b77

This closes apache#146. Signed-off-by: Marc Parisi <phrocker@apache.org>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINIFICPP-72: Add Tar and Zip Support for MergeContent #146

MINIFICPP-72: Add Tar and Zip Support for MergeContent #146

minifirocks commented Oct 9, 2017

phrocker Oct 9, 2017

minifirocks Oct 9, 2017

phrocker Oct 9, 2017

minifirocks Oct 9, 2017

phrocker Oct 9, 2017

minifirocks Oct 11, 2017

phrocker Oct 11, 2017

minifirocks Oct 11, 2017

phrocker Oct 9, 2017

minifirocks Oct 9, 2017

minifirocks Oct 16, 2017

phrocker Oct 9, 2017

minifirocks Oct 11, 2017

phrocker Oct 9, 2017

minifirocks Oct 9, 2017

minifirocks Oct 9, 2017

phrocker Oct 10, 2017

minifirocks commented Oct 10, 2017

minifirocks commented Oct 11, 2017

apiri left a comment

apiri Oct 12, 2017

minifirocks commented Oct 12, 2017

minifirocks commented Oct 13, 2017

apiri left a comment

apiri Oct 16, 2017

apiri Oct 16, 2017

minifirocks Oct 16, 2017

minifirocks Oct 16, 2017

minifirocks commented Oct 18, 2017

phrocker left a comment

		(END LICENSE TEXT)

		Each individual file in this distribution should have a clear

		@@ -564,4 +564,68 @@ ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
		CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE

MINIFICPP-72: Add Tar and Zip Support for MergeContent #146

MINIFICPP-72: Add Tar and Zip Support for MergeContent #146

Conversation

minifirocks commented Oct 9, 2017

For all changes:

For code changes:

For documentation related changes:

Note:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minifirocks commented Oct 10, 2017

minifirocks commented Oct 11, 2017

apiri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minifirocks commented Oct 12, 2017

minifirocks commented Oct 13, 2017

apiri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minifirocks commented Oct 18, 2017

phrocker left a comment

Choose a reason for hiding this comment