You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "martinzink (via GitHub)" <gi...@apache.org> on 2024/10/10 08:03:56 UTC
[PR] MINIFICPP-2463 TailFile doesnt pickup the data after the last delimit… [nifi-minifi-cpp]
martinzink opened a new pull request, #1877:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1877
…er in rotated files
The actual change amidst the many clangtidy fixes and TailFile test optimizations (runtime went from ~6s to ~1.6s):
I've added a new is_rotated_: bool field to TailState, thats only set to true during the findRotatedFilesAfterLastReadTime fn, and we not only commit the split flowfile if its ended with delimiter but also if its rotated and not empty
---
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
Re: [PR] MINIFICPP-2463 TailFile doesnt pickup the data after the last delimit… [nifi-minifi-cpp]
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1877:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1877#discussion_r1843644673
##########
extensions/standard-processors/tests/unit/TailFileTests.cpp:
##########
Review Comment:
There is no new test however I modified the `TEST_CASE("TailFile finds and finishes the renamed file and continues with the new log file", "[rotation]")` removed the deliberatly added delimiter after the rotation, so now it would fail without the fix
previously line 820
```
in_file_stream << DELIM;
in_file_stream.close();
```
--
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
Re: [PR] MINIFICPP-2463 TailFile doesnt pickup the data after the last delimit… [nifi-minifi-cpp]
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1877:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1877#discussion_r1806138742
##########
extensions/standard-processors/processors/TailFile.cpp:
##########
@@ -695,8 +694,8 @@ void TailFile::processSingleFile(core::ProcessSession& session,
auto flow_file = session.create();
session.write(flow_file, std::ref(file_reader));
- if (file_reader.useLatestFlowFile()) {
- updateFlowFileAttributes(full_file_name, state_copy, fileName, baseName, extension, flow_file);
+ if (file_reader.endedWithDelimiter() || (state.is_rotated_ && flow_file->getSize() > 0)) {
Review Comment:
yeah
> The actual change amidst the many clangtidy fixes and TailFile test optimizations (runtime went from ~6s to ~1.6s):
>
> I've added a new is_rotated_: bool field to TailState, thats only set to true during the findRotatedFilesAfterLastReadTime fn, and we not only commit the split flowfile if its ended with delimiter but also if its rotated and not empty
--
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
Re: [PR] MINIFICPP-2463 TailFile doesnt pickup the data after the last delimit… [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1877:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1877#discussion_r1795658596
##########
extensions/standard-processors/processors/TailFile.cpp:
##########
@@ -724,24 +723,24 @@ void TailFile::processSingleFile(core::ProcessSession& session,
void TailFile::updateFlowFileAttributes(const std::filesystem::path& full_file_name, const TailState& state,
const std::filesystem::path& fileName, const std::string& baseName,
const std::string& extension,
- std::shared_ptr<core::FlowFile> &flow_file) const {
- logger_->log_info("TailFile {} for {} bytes", fileName, flow_file->getSize());
- std::string logName = textfragmentutils::createFileName(baseName, extension, state.position_, flow_file->getSize());
- flow_file->setAttribute(core::SpecialFlowAttribute::PATH, state.path_.string());
- flow_file->addAttribute(core::SpecialFlowAttribute::ABSOLUTE_PATH, full_file_name.string());
- flow_file->setAttribute(core::SpecialFlowAttribute::FILENAME, logName);
+ core::FlowFile& flow_file) const {
Review Comment:
Interesting way to update the Flow File attributes switching from shared pointer to pass by reference. I see you also dereferenced the pointer as you passed *flow_file into updateFlowFileAttributes. Were you noticing problems with the shared pointer pass by reference previous approach?
--
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
Re: [PR] MINIFICPP-2463 TailFile doesnt pickup the data after the last delimit… [nifi-minifi-cpp]
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1877:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1877#discussion_r1820339212
##########
extensions/standard-processors/processors/TailFile.h:
##########
@@ -253,21 +253,21 @@ class TailFile : public core::Processor {
const std::filesystem::path& full_file_name,
TailState &state);
bool getStateFromStateManager(std::map<std::filesystem::path, TailState> &new_tail_states) const;
- bool getStateFromLegacyStateFile(core::ProcessContext& context,
+ bool getStateFromLegacyStateFile(const core::ProcessContext& context,
std::map<std::filesystem::path, TailState> &new_tail_states) const;
void doMultifileLookup(core::ProcessContext& context);
void checkForRemovedFiles();
void checkForNewFiles(core::ProcessContext& context);
static std::string baseDirectoryFromAttributes(const controllers::AttributeProviderService::AttributeMap& attribute_map, core::ProcessContext& context);
void updateFlowFileAttributes(const std::filesystem::path& full_file_name, const TailState &state, const std::filesystem::path& fileName,
const std::string &baseName, const std::string &extension,
- std::shared_ptr<core::FlowFile> &flow_file) const;
+ core::FlowFile& flow_file) const;
static void updateStateAttributes(TailState &state, uint64_t size, uint64_t checksum);
- bool isOldFileInitiallyRead(TailState &state) const;
+ bool isOldFileInitiallyRead(const TailState &state) const;
static const char *CURRENT_STR;
static const char *POSITION_STR;
- static const int BUFFER_SIZE = 512;
+ static constexpr int BUFFER_SIZE = 512;
Review Comment:
Good idea but I rather do that in a separate PR, I've created a jira so we can address all of these inner buffer sizes together (possibly making them configurable) https://issues.apache.org/jira/browse/MINIFICPP-2483
--
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
Re: [PR] MINIFICPP-2463 TailFile doesnt pickup the data after the last delimit… [nifi-minifi-cpp]
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1877:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1877#discussion_r1799123177
##########
extensions/standard-processors/processors/TailFile.cpp:
##########
@@ -724,24 +723,24 @@ void TailFile::processSingleFile(core::ProcessSession& session,
void TailFile::updateFlowFileAttributes(const std::filesystem::path& full_file_name, const TailState& state,
const std::filesystem::path& fileName, const std::string& baseName,
const std::string& extension,
- std::shared_ptr<core::FlowFile> &flow_file) const {
- logger_->log_info("TailFile {} for {} bytes", fileName, flow_file->getSize());
- std::string logName = textfragmentutils::createFileName(baseName, extension, state.position_, flow_file->getSize());
- flow_file->setAttribute(core::SpecialFlowAttribute::PATH, state.path_.string());
- flow_file->addAttribute(core::SpecialFlowAttribute::ABSOLUTE_PATH, full_file_name.string());
- flow_file->setAttribute(core::SpecialFlowAttribute::FILENAME, logName);
+ core::FlowFile& flow_file) const {
Review Comment:
No particular problems but this makes the signature more clear and removes unnecessary restrictions, so this could help future refactors.
- this function could easily work on flowfiles owned by unique_ptrs or flowfiles on the stack
- it also ensures that the flowfile is not supposed to be a nullptr and moves the responsibility to ensure it is not null to the caller side (when it deferences the shared_ptr for example)
--
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
Re: [PR] MINIFICPP-2463 TailFile doesnt pickup the data after the last delimit… [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #1877:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1877#discussion_r1838443687
##########
extensions/standard-processors/tests/unit/TailFileTests.cpp:
##########
Review Comment:
The refactoring changes all look like they are improvements, but they make it difficult to see if there is a specific new test which validates the bugfix. Is there one?
--
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
Re: [PR] MINIFICPP-2463 TailFile doesnt pickup the data after the last delimit… [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm closed pull request #1877: MINIFICPP-2463 TailFile doesnt pickup the data after the last delimit…
URL: https://github.com/apache/nifi-minifi-cpp/pull/1877
--
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
Re: [PR] MINIFICPP-2463 TailFile doesnt pickup the data after the last delimit… [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #1877:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1877#discussion_r1843694224
##########
extensions/standard-processors/tests/unit/TailFileTests.cpp:
##########
Review Comment:
OK, thanks!
--
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
Re: [PR] MINIFICPP-2463 TailFile doesnt pickup the data after the last delimit… [nifi-minifi-cpp]
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1877:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1877#discussion_r1799123177
##########
extensions/standard-processors/processors/TailFile.cpp:
##########
@@ -724,24 +723,24 @@ void TailFile::processSingleFile(core::ProcessSession& session,
void TailFile::updateFlowFileAttributes(const std::filesystem::path& full_file_name, const TailState& state,
const std::filesystem::path& fileName, const std::string& baseName,
const std::string& extension,
- std::shared_ptr<core::FlowFile> &flow_file) const {
- logger_->log_info("TailFile {} for {} bytes", fileName, flow_file->getSize());
- std::string logName = textfragmentutils::createFileName(baseName, extension, state.position_, flow_file->getSize());
- flow_file->setAttribute(core::SpecialFlowAttribute::PATH, state.path_.string());
- flow_file->addAttribute(core::SpecialFlowAttribute::ABSOLUTE_PATH, full_file_name.string());
- flow_file->setAttribute(core::SpecialFlowAttribute::FILENAME, logName);
+ core::FlowFile& flow_file) const {
Review Comment:
No particular problems but this makes the signature more clear and removes unnecessary restrictions, so this could help future refactors.
this function could easily work on flowfiles owned by unique_ptrs or flowfiles on the stack
it also ensures that the flowfile is not supposed to be a nullptr and moves the responsibility to ensure it is not null to the caller side (when it deferences the shared_ptr for example)
--
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
Re: [PR] MINIFICPP-2463 TailFile doesnt pickup the data after the last delimit… [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1877:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1877#discussion_r1801154844
##########
extensions/standard-processors/processors/TailFile.h:
##########
@@ -253,21 +253,21 @@ class TailFile : public core::Processor {
const std::filesystem::path& full_file_name,
TailState &state);
bool getStateFromStateManager(std::map<std::filesystem::path, TailState> &new_tail_states) const;
- bool getStateFromLegacyStateFile(core::ProcessContext& context,
+ bool getStateFromLegacyStateFile(const core::ProcessContext& context,
std::map<std::filesystem::path, TailState> &new_tail_states) const;
void doMultifileLookup(core::ProcessContext& context);
void checkForRemovedFiles();
void checkForNewFiles(core::ProcessContext& context);
static std::string baseDirectoryFromAttributes(const controllers::AttributeProviderService::AttributeMap& attribute_map, core::ProcessContext& context);
void updateFlowFileAttributes(const std::filesystem::path& full_file_name, const TailState &state, const std::filesystem::path& fileName,
const std::string &baseName, const std::string &extension,
- std::shared_ptr<core::FlowFile> &flow_file) const;
+ core::FlowFile& flow_file) const;
static void updateStateAttributes(TailState &state, uint64_t size, uint64_t checksum);
- bool isOldFileInitiallyRead(TailState &state) const;
+ bool isOldFileInitiallyRead(const TailState &state) const;
static const char *CURRENT_STR;
static const char *POSITION_STR;
- static const int BUFFER_SIZE = 512;
+ static constexpr int BUFFER_SIZE = 512;
Review Comment:
We could increase this to improve performance
##########
extensions/standard-processors/processors/TailFile.cpp:
##########
@@ -695,8 +694,8 @@ void TailFile::processSingleFile(core::ProcessSession& session,
auto flow_file = session.create();
session.write(flow_file, std::ref(file_reader));
- if (file_reader.useLatestFlowFile()) {
- updateFlowFileAttributes(full_file_name, state_copy, fileName, baseName, extension, flow_file);
+ if (file_reader.endedWithDelimiter() || (state.is_rotated_ && flow_file->getSize() > 0)) {
Review Comment:
is this the fix?
##########
extensions/standard-processors/tests/unit/TailFileTests.cpp:
##########
@@ -18,138 +18,112 @@
#include <cstdio>
#include <fstream>
-#include <map>
-#include <memory>
-#include <utility>
#include <string>
#include <iostream>
#include <set>
#include <algorithm>
#include <random>
-#include <cstdlib>
#include "FlowController.h"
#include "unit/TestBase.h"
#include "unit/Catch.h"
#include "core/Core.h"
-#include "core/FlowFile.h"
#include "utils/file/FileUtils.h"
#include "utils/file/PathUtils.h"
#include "unit/ProvenanceTestHelper.h"
#include "core/Processor.h"
#include "core/ProcessContext.h"
#include "core/ProcessSession.h"
-#include "core/ProcessorNode.h"
#include "core/Resource.h"
#include "TailFile.h"
#include "LogAttribute.h"
#include "unit/TestUtils.h"
#include "utils/StringUtils.h"
#include "unit/SingleProcessorTestController.h"
+#include "TextFragmentUtils.h"
using namespace std::literals::chrono_literals;
-static const std::string NEWLINE_FILE = "" // NOLINT
+static constexpr std::string_view NEWLINE_FILE = "" // NOLINT
"one,two,three\n"
"four,five,six, seven";
-static const char *TMP_FILE = "minifi-tmpfile.txt";
-static const char *ROLLED_OVER_TMP_FILE = "minifi-tmpfile.txt.old.1";
-static const char *STATE_FILE = "minifi-state-file.txt";
-static const std::string ROLLED_OVER_TAIL_DATA = "rolled_over_data\n";
-static const std::string NEW_TAIL_DATA = "newdata\n";
-static const std::string ADDITIONALY_CREATED_FILE_CONTENT = "additional file data\n";
+static constexpr std::string_view TMP_FILE = "minifi-tmpfile.txt";
+static constexpr std::string_view ROLLED_OVER_TMP_FILE = "minifi-tmpfile.txt.old.1";
+static constexpr std::string_view STATE_FILE = "minifi-state-file.txt";
+static constexpr std::string_view ROLLED_OVER_TAIL_DATA = "rolled_over_data\n";
+static constexpr std::string_view NEW_TAIL_DATA = "newdata\n";
+static constexpr std::string_view ADDITIONALY_CREATED_FILE_CONTENT = "additional file data\n";
namespace {
-std::filesystem::path createTempFile(const std::filesystem::path& directory, const std::filesystem::path& file_name, const std::string& contents,
- std::ios_base::openmode open_mode = std::ios::out | std::ios::binary) {
+std::filesystem::path createTempFile(const std::filesystem::path& directory, const std::filesystem::path& file_name,
+ const std::string_view contents, const std::ios_base::openmode open_mode = std::ios::out | std::ios::binary,
+ const std::chrono::file_clock::duration offset = 0ms) {
if (!utils::file::exists(directory)) {
std::filesystem::create_directories(directory);
}
- std::string full_file_name = (directory / file_name).string();
- std::ofstream tmpfile{full_file_name, open_mode};
- tmpfile << contents;
+ auto full_file_name = directory / file_name;
+ {
+ std::ofstream tmp_file{full_file_name, open_mode};
+ tmp_file << contents;
+ }
Review Comment:
A bit of nitpick, but I would have written it like this:
```suggestion
std::ofstream{full_file_name, open_mode} << contents;
```
But the current version is fine too, feel free to close this if you like it better.
--
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
Re: [PR] MINIFICPP-2463 TailFile doesnt pickup the data after the last delimit… [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1877:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1877#discussion_r1801151878
##########
extensions/standard-processors/processors/TailFile.cpp:
##########
@@ -724,24 +723,24 @@ void TailFile::processSingleFile(core::ProcessSession& session,
void TailFile::updateFlowFileAttributes(const std::filesystem::path& full_file_name, const TailState& state,
const std::filesystem::path& fileName, const std::string& baseName,
const std::string& extension,
- std::shared_ptr<core::FlowFile> &flow_file) const {
- logger_->log_info("TailFile {} for {} bytes", fileName, flow_file->getSize());
- std::string logName = textfragmentutils::createFileName(baseName, extension, state.position_, flow_file->getSize());
- flow_file->setAttribute(core::SpecialFlowAttribute::PATH, state.path_.string());
- flow_file->addAttribute(core::SpecialFlowAttribute::ABSOLUTE_PATH, full_file_name.string());
- flow_file->setAttribute(core::SpecialFlowAttribute::FILENAME, logName);
+ core::FlowFile& flow_file) const {
Review Comment:
@james94 passing everything by shared_ptr is a half decade old legacy of the codebase that we're trying to reduce. In most cases, we can assign each object to a clear owner, with unique_ptr, which makes it easier to reason about lifetimes, and ensures that we don't accidentally create shared_ptr ownership cycles like [MINIFICPP-839](https://issues.apache.org/jira/browse/MINIFICPP-839)
And when something doesn't need ownership, it shouldn't get ownership. :)
--
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
Re: [PR] MINIFICPP-2463 TailFile doesnt pickup the data after the last delimit… [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1877:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1877#discussion_r1801151878
##########
extensions/standard-processors/processors/TailFile.cpp:
##########
@@ -724,24 +723,24 @@ void TailFile::processSingleFile(core::ProcessSession& session,
void TailFile::updateFlowFileAttributes(const std::filesystem::path& full_file_name, const TailState& state,
const std::filesystem::path& fileName, const std::string& baseName,
const std::string& extension,
- std::shared_ptr<core::FlowFile> &flow_file) const {
- logger_->log_info("TailFile {} for {} bytes", fileName, flow_file->getSize());
- std::string logName = textfragmentutils::createFileName(baseName, extension, state.position_, flow_file->getSize());
- flow_file->setAttribute(core::SpecialFlowAttribute::PATH, state.path_.string());
- flow_file->addAttribute(core::SpecialFlowAttribute::ABSOLUTE_PATH, full_file_name.string());
- flow_file->setAttribute(core::SpecialFlowAttribute::FILENAME, logName);
+ core::FlowFile& flow_file) const {
Review Comment:
@james94 passing everything by shared_ptr is a half decade old legacy of the codebase that we're trying to reduce. In most cases, we can assign each object to a clear owner, with unique_ptr, which makes it easier to reason about lifetimes, and ensures that we don't accidentally create shared_ptr ownership cycles like [MINIFICPP-839](https://issues.apache.org/jira/browse/MINIFICPP-839)
--
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