You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/09/20 07:36:21 UTC

[GitHub] [nifi-minifi-cpp] martinzink opened a new pull request #1177: MINIFICPP-1644: ProcessSession::read ignores the size/offset of the

martinzink opened a new pull request #1177:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1177


   flowfile
   
   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 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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1177: MINIFICPP-1644: ProcessSession::read ignores the size/offset of the

Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1177:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1177#discussion_r714719344



##########
File path: libminifi/include/io/AtomicEntryStream.h
##########
@@ -108,6 +112,11 @@ void AtomicEntryStream<T>::seek(size_t offset) {
   offset_ = gsl::narrow<size_t>(offset);
 }
 
+template<typename T>
+size_t AtomicEntryStream<T>::tell() const {
+  return offset_;
+}

Review comment:
       changed in https://github.com/apache/nifi-minifi-cpp/pull/1177/commits/0a442efb33954be698d0ed5daa5148ab5f3bd02b




-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1177: MINIFICPP-1644: ProcessSession::read ignores the size/offset of the

Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1177:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1177#discussion_r712094100



##########
File path: libminifi/src/core/ProcessSession.cpp
##########
@@ -311,9 +333,8 @@ int64_t ProcessSession::read(const std::shared_ptr<core::FlowFile> &flow, InputS
       throw Exception(FILE_OPERATION_EXCEPTION, "Failed to open flowfile content for read");
     }
 
-    stream->seek(flow->getOffset());
-
-    auto ret = callback->process(stream);
+    auto flow_file_stream = std::make_shared<FlowFileStream>(stream, flow);
+    auto ret = callback->process(flow_file_stream);

Review comment:
       The size() function is not delegated thats the whole reason of the wrapper, it uses the flowfile size and not the stream size.




-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1177: MINIFICPP-1644: ProcessSession::read ignores the size/offset of the

Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1177:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1177#discussion_r716551528



##########
File path: libminifi/include/io/StreamSlice.h
##########
@@ -0,0 +1,64 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+#include <algorithm>
+#include <memory>
+
+#include "BaseStream.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace io {

Review comment:
       awesome, didnt know that feature of C++17 but it looks way cleaner
   changed it in https://github.com/apache/nifi-minifi-cpp/pull/1177/commits/35b1e605cbc4760b9f1004f2dae045acdd0a5556




-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1177: MINIFICPP-1644: ProcessSession::read ignores the size/offset of the

Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1177:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1177#discussion_r712095269



##########
File path: libminifi/src/core/ProcessSession.cpp
##########
@@ -311,9 +333,8 @@ int64_t ProcessSession::read(const std::shared_ptr<core::FlowFile> &flow, InputS
       throw Exception(FILE_OPERATION_EXCEPTION, "Failed to open flowfile content for read");
     }
 
-    stream->seek(flow->getOffset());
-
-    auto ret = callback->process(stream);
+    auto flow_file_stream = std::make_shared<FlowFileStream>(stream, flow);
+    auto ret = callback->process(flow_file_stream);

Review comment:
       Oh, I missed that detail. Nevermind then, thanks for clarifying this. Could you maybe add this as a comment above the class?




-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] adamdebreceni closed pull request #1177: MINIFICPP-1644: ProcessSession::read ignores the size/offset of the

Posted by GitBox <gi...@apache.org>.
adamdebreceni closed pull request #1177:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1177


   


-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1177: MINIFICPP-1644: ProcessSession::read ignores the size/offset of the

Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1177:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1177#discussion_r714528530



##########
File path: libminifi/src/core/ProcessSession.cpp
##########
@@ -290,6 +290,33 @@ void ProcessSession::append(const std::shared_ptr<core::FlowFile> &flow, OutputS
   }
 }
 
+namespace {
+// FlowFileStream uses the flowfile's size instead of the stream size (so it will not be over-read)
+class FlowFileStream : public io::BaseStream {

Review comment:
       the only thing we use from the flowfile is the offset and size, I don't think we need to store the flowfile, we could generalize this `FlowFileStream` to a `StreamSlice` taking an offset and size




-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] adamdebreceni closed pull request #1177: MINIFICPP-1644: ProcessSession::read ignores the size/offset of the

Posted by GitBox <gi...@apache.org>.
adamdebreceni closed pull request #1177:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1177


   


-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1177: MINIFICPP-1644: ProcessSession::read ignores the size/offset of the

Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1177:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1177#discussion_r716508218



##########
File path: libminifi/include/io/StreamSlice.h
##########
@@ -0,0 +1,64 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+#include <algorithm>
+#include <memory>
+
+#include "BaseStream.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace io {

Review comment:
       from c++17, we can use the nested namespace definition `namespace org::apache::nifi::minifi::io {`




-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] martinzink commented on pull request #1177: MINIFICPP-1644: ProcessSession::read ignores the size/offset of the

Posted by GitBox <gi...@apache.org>.
martinzink commented on pull request #1177:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1177#issuecomment-924047824


   The current read callbacks work either
   - by reading until the size of the stream, this coulld be fixed by wrapping the size() function.
   - or they read batch-by-batch until the read returns 0 (e.g. PutFile::ReadCallback::process), for this I've wrapped the read function so it wont over-read 
   
   For this I had to add tell function to `minifi::io::Stream` which will tell the current seek position
   I implemented `tell` everywhere where `seek` was implemented.
   


-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1177: MINIFICPP-1644: ProcessSession::read ignores the size/offset of the

Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1177:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1177#discussion_r713772532



##########
File path: libminifi/include/io/AtomicEntryStream.h
##########
@@ -108,6 +112,11 @@ void AtomicEntryStream<T>::seek(size_t offset) {
   offset_ = gsl::narrow<size_t>(offset);
 }
 
+template<typename T>
+size_t AtomicEntryStream<T>::tell() const {
+  return offset_;
+}

Review comment:
       I would move this inline in the class definition. The implementation is trivial, so it wouldn't hurt readability IMO.




-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1177: MINIFICPP-1644: ProcessSession::read ignores the size/offset of the

Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1177:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1177#discussion_r712846068



##########
File path: libminifi/test/unit/ContentRepositoryDependentTests.h
##########
@@ -0,0 +1,119 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include <catch.hpp>
+#include "core/ProcessSession.h"
+#include "core/Resource.h"
+#include "../TestBase.h"
+#include "StreamPipe.h"
+
+#pragma once
+
+namespace ContentRepositoryDependentTests {
+
+struct WriteStringToFlowFile : public minifi::OutputStreamCallback {
+  const std::vector<uint8_t> buffer_;
+
+  explicit WriteStringToFlowFile(const std::string& buffer) : buffer_(buffer.begin(), buffer.end()) {}
+
+  int64_t process(const std::shared_ptr<minifi::io::BaseStream> &stream) override {
+    size_t bytes_written = stream->write(buffer_, buffer_.size());
+    return minifi::io::isError(bytes_written) ? -1 : gsl::narrow<int64_t>(bytes_written);
+  }
+};
+
+struct ReadFlowFileIntoString : public minifi::InputStreamCallback {
+  std::string value_;
+
+  int64_t process(const std::shared_ptr<minifi::io::BaseStream> &stream) override {
+    value_.clear();
+    std::vector<uint8_t> buffer;
+    size_t bytes_read = stream->read(buffer, stream->size());
+    value_.assign(buffer.begin(), buffer.end());
+    return minifi::io::isError(bytes_read) ? -1 : gsl::narrow<int64_t>(bytes_read);
+  }
+};
+
+class DummyProcessor : public core::Processor {
+  using core::Processor::Processor;
+};
+
+REGISTER_RESOURCE(DummyProcessor, "A processor that does nothing.");
+
+template<class ContentRepositoryClass>
+class Fixture {

Review comment:
       changed it in https://github.com/apache/nifi-minifi-cpp/pull/1177/commits/90c079e297e087388c36f6a72301f4630c942a27 , however I found additional related bugs, I will convert the PR to draft and update it




-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] martinzink commented on pull request #1177: MINIFICPP-1644: ProcessSession::read ignores the size/offset of the

Posted by GitBox <gi...@apache.org>.
martinzink commented on pull request #1177:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1177#issuecomment-925744495


   After consulting with @adamdebreceni I also changed the BufferStream::seek() to function as an absolute seeker (like the other stream implementations) and not like a relative seeker.


-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1177: MINIFICPP-1644: ProcessSession::read ignores the size/offset of the

Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1177:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1177#discussion_r712118674



##########
File path: libminifi/test/TestBase.h
##########
@@ -427,12 +427,18 @@ class TestController {
     flow_version_ = std::make_shared<minifi::state::response::FlowVersion>("test", "test", "test");
   }
 
+  std::shared_ptr<TestPlan> createPlan(std::shared_ptr<minifi::Configure> configuration = nullptr, const char* state_dir = nullptr) {
+    return createPlan<core::repository::VolatileContentRepository>(configuration, state_dir);
+  }
+
+  template<class ContentRepositoryClass>
   std::shared_ptr<TestPlan> createPlan(std::shared_ptr<minifi::Configure> configuration = nullptr, const char* state_dir = nullptr) {
     if (configuration == nullptr) {
       configuration = std::make_shared<minifi::Configure>();
       configuration->set(minifi::Configure::nifi_state_management_provider_local_class_name, "UnorderedMapKeyValueStoreService");
+      configuration->set(minifi::Configure::nifi_dbcontent_repository_directory_default, createTempDirectory());
     }
-    std::shared_ptr<core::ContentRepository> content_repo = std::make_shared<core::repository::VolatileContentRepository>();
+    std::shared_ptr<core::ContentRepository> content_repo = std::make_shared<ContentRepositoryClass>();

Review comment:
       Good idea changed it in https://github.com/apache/nifi-minifi-cpp/pull/1177/commits/3026364fda3a6b5b6512a3ba2c3b0a6dccfd7501

##########
File path: libminifi/src/core/ProcessSession.cpp
##########
@@ -311,9 +333,8 @@ int64_t ProcessSession::read(const std::shared_ptr<core::FlowFile> &flow, InputS
       throw Exception(FILE_OPERATION_EXCEPTION, "Failed to open flowfile content for read");
     }
 
-    stream->seek(flow->getOffset());
-
-    auto ret = callback->process(stream);
+    auto flow_file_stream = std::make_shared<FlowFileStream>(stream, flow);
+    auto ret = callback->process(flow_file_stream);

Review comment:
       Sure thing, that would make it clearer. added in https://github.com/apache/nifi-minifi-cpp/pull/1177/commits/3026364fda3a6b5b6512a3ba2c3b0a6dccfd7501




-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1177: MINIFICPP-1644: ProcessSession::read ignores the size/offset of the

Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1177:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1177#discussion_r712137870



##########
File path: libminifi/test/unit/ContentRepositoryDependentTests.h
##########
@@ -0,0 +1,119 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include <catch.hpp>
+#include "core/ProcessSession.h"
+#include "core/Resource.h"
+#include "../TestBase.h"
+#include "StreamPipe.h"
+
+#pragma once
+
+namespace ContentRepositoryDependentTests {
+
+struct WriteStringToFlowFile : public minifi::OutputStreamCallback {
+  const std::vector<uint8_t> buffer_;
+
+  explicit WriteStringToFlowFile(const std::string& buffer) : buffer_(buffer.begin(), buffer.end()) {}
+
+  int64_t process(const std::shared_ptr<minifi::io::BaseStream> &stream) override {
+    size_t bytes_written = stream->write(buffer_, buffer_.size());
+    return minifi::io::isError(bytes_written) ? -1 : gsl::narrow<int64_t>(bytes_written);
+  }
+};
+
+struct ReadFlowFileIntoString : public minifi::InputStreamCallback {
+  std::string value_;
+
+  int64_t process(const std::shared_ptr<minifi::io::BaseStream> &stream) override {
+    value_.clear();
+    std::vector<uint8_t> buffer;
+    size_t bytes_read = stream->read(buffer, stream->size());
+    value_.assign(buffer.begin(), buffer.end());
+    return minifi::io::isError(bytes_read) ? -1 : gsl::narrow<int64_t>(bytes_read);
+  }
+};
+
+class DummyProcessor : public core::Processor {
+  using core::Processor::Processor;
+};
+
+REGISTER_RESOURCE(DummyProcessor, "A processor that does nothing.");
+
+template<class ContentRepositoryClass>
+class Fixture {

Review comment:
       I would make the same modifications here as I suggested for createPlan: Keep things simple and take a content repo as parameter instead of making the whole thing a template.
   In this case, I'm thinking about a constructor parameter for `Fixture` and a function parameter for `testReadOnSmallerClonedFlowFiles`.




-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] martinzink edited a comment on pull request #1177: MINIFICPP-1644: ProcessSession::read ignores the size/offset of the

Posted by GitBox <gi...@apache.org>.
martinzink edited a comment on pull request #1177:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1177#issuecomment-924047824


   The current read callbacks work either
   - by reading until the size of the stream (e.g. utils::ByteOutputCallback::process), this coulld be fixed by wrapping the size() function.
   - or they read batch-by-batch until the read returns 0 (e.g. PutFile::ReadCallback::process), for this I've wrapped the read function so it wont over-read 
   
   For this I had to add tell function to `minifi::io::Stream` which will tell the current seek position
   I implemented `tell` everywhere where `seek` was implemented.
   


-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1177: MINIFICPP-1644: ProcessSession::read ignores the size/offset of the

Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1177:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1177#discussion_r712013951



##########
File path: libminifi/src/core/ProcessSession.cpp
##########
@@ -311,9 +333,8 @@ int64_t ProcessSession::read(const std::shared_ptr<core::FlowFile> &flow, InputS
       throw Exception(FILE_OPERATION_EXCEPTION, "Failed to open flowfile content for read");
     }
 
-    stream->seek(flow->getOffset());
-
-    auto ret = callback->process(stream);
+    auto flow_file_stream = std::make_shared<FlowFileStream>(stream, flow);
+    auto ret = callback->process(flow_file_stream);

Review comment:
       Now that you made RocksDbStream seekable, FlowFileStream seems unnecessary. All it does is call seek in the constructor and delegate everything, so simply calling seek is enough IMO.

##########
File path: libminifi/test/TestBase.h
##########
@@ -427,12 +427,18 @@ class TestController {
     flow_version_ = std::make_shared<minifi::state::response::FlowVersion>("test", "test", "test");
   }
 
+  std::shared_ptr<TestPlan> createPlan(std::shared_ptr<minifi::Configure> configuration = nullptr, const char* state_dir = nullptr) {
+    return createPlan<core::repository::VolatileContentRepository>(configuration, state_dir);
+  }
+
+  template<class ContentRepositoryClass>
   std::shared_ptr<TestPlan> createPlan(std::shared_ptr<minifi::Configure> configuration = nullptr, const char* state_dir = nullptr) {
     if (configuration == nullptr) {
       configuration = std::make_shared<minifi::Configure>();
       configuration->set(minifi::Configure::nifi_state_management_provider_local_class_name, "UnorderedMapKeyValueStoreService");
+      configuration->set(minifi::Configure::nifi_dbcontent_repository_directory_default, createTempDirectory());
     }
-    std::shared_ptr<core::ContentRepository> content_repo = std::make_shared<core::repository::VolatileContentRepository>();
+    std::shared_ptr<core::ContentRepository> content_repo = std::make_shared<ContentRepositoryClass>();

Review comment:
       Instead of making the whole function a template, I think it would be simpler to make the content repository an optional parameter. 




-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1177: MINIFICPP-1644: ProcessSession::read ignores the size/offset of the

Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1177:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1177#discussion_r714722272



##########
File path: libminifi/src/core/ProcessSession.cpp
##########
@@ -290,6 +290,33 @@ void ProcessSession::append(const std::shared_ptr<core::FlowFile> &flow, OutputS
   }
 }
 
+namespace {
+// FlowFileStream uses the flowfile's size instead of the stream size (so it will not be over-read)
+class FlowFileStream : public io::BaseStream {

Review comment:
       Good idea.
   I also changed seek() to work properly from the beginning of the slice (instead of the stream).
   
   Added also a couple more unit tests. https://github.com/apache/nifi-minifi-cpp/pull/1177/commits/0a442efb33954be698d0ed5daa5148ab5f3bd02b




-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org