[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #791: MINIFICPP-1177 Improvements to the TailFile processor

[GitBox <gi...@apache.org>]

arpadboda commented on a change in pull request #791:
URL: https://github.com/apache/nifi-minifi-cpp/pull/791#discussion_r428734124



##########
File path: extensions/standard-processors/processors/TailFile.cpp
##########
@@ -120,56 +158,42 @@ void TailFile::onSchedule(const std::shared_ptr<core::ProcessContext> &context,
   std::string value;
 
   if (context->getProperty(Delimiter.getName(), value)) {
-    delimiter_ = value;
+    delimiter_ = parseDelimiter(value);
+  }
+
+  if (!context->getProperty(FileName.getName(), file_to_tail_)) {
+    throw minifi::Exception(ExceptionType::PROCESSOR_EXCEPTION, "File to Tail is a required property");
   }
 
   std::string mode;
   context->getProperty(TailMode.getName(), mode);
 
-  std::string file = "";
-  if (!context->getProperty(FileName.getName(), file)) {
-    throw minifi::Exception(ExceptionType::PROCESSOR_EXCEPTION, "File to Tail is a required property");
-  }
   if (mode == "Multiple file") {
-    // file is a regex
-    std::string base_dir;
-    if (!context->getProperty(BaseDirectory.getName(), base_dir)) {
+    tail_mode_ = Mode::MULTIPLE;
+
+    if (!context->getProperty(BaseDirectory.getName(), base_dir_)) {
       throw minifi::Exception(ExceptionType::PROCESSOR_EXCEPTION, "Base directory is required for multiple tail mode.");
     }
 
-    auto fileRegexSelect = [&](const std::string& path, const std::string& filename) -> bool {
-      if (acceptFile(file, filename)) {
-        tail_states_.insert(std::make_pair(filename, TailState {path, filename, 0, 0}));
-      }
-      return true;
-    };
-
-    utils::file::FileUtils::list_dir(base_dir, fileRegexSelect, logger_, false);
+    // in multiple mode, we check for new/removed files in every onTrigger
 
   } else {
+    tail_mode_ = Mode::SINGLE;
+
     std::string fileLocation, fileName;
-    if (utils::file::PathUtils::getFileNameAndPath(file, fileLocation, fileName)) {
-      tail_states_.insert(std::make_pair(fileName, TailState { fileLocation, fileName, 0, 0 }));
+    if (utils::file::PathUtils::getFileNameAndPath(file_to_tail_, fileLocation, fileName)) {
+      tail_states_.emplace(fileName, TailState{fileLocation, fileName, 0, 0, 0, 0});

Review comment:
       Are we sure about this? 
   As far as I understand the change this means that whenever MiNiFi is restarted, the state is dropped and we start reading the file from the beginning. 

##########
File path: extensions/standard-processors/processors/TailFile.cpp
##########
@@ -200,52 +224,65 @@ void TailFile::parseStateFileLine(char *buf) {
   }
 
   std::string value = equal;
-  key = trimRight(key);
-  value = trimRight(value);
+  key = utils::StringUtils::trimRight(key);
+  value = utils::StringUtils::trimRight(value);
 
   if (key == "FILENAME") {
     std::string fileLocation, fileName;
     if (utils::file::PathUtils::getFileNameAndPath(value, fileLocation, fileName)) {
       logger_->log_debug("State migration received path %s, file %s", fileLocation, fileName);
-      tail_states_.insert(std::make_pair(fileName, TailState { fileLocation, fileName, 0, 0 }));
+      state.insert(std::make_pair(fileName, TailState{fileLocation, fileName, 0, 0, 0, 0}));

Review comment:
       Using emplace we can avoid making pair. 

##########
File path: extensions/standard-processors/processors/TailFile.cpp
##########
@@ -120,56 +158,42 @@ void TailFile::onSchedule(const std::shared_ptr<core::ProcessContext> &context,
   std::string value;
 
   if (context->getProperty(Delimiter.getName(), value)) {
-    delimiter_ = value;
+    delimiter_ = parseDelimiter(value);
+  }
+
+  if (!context->getProperty(FileName.getName(), file_to_tail_)) {
+    throw minifi::Exception(ExceptionType::PROCESSOR_EXCEPTION, "File to Tail is a required property");
   }
 
   std::string mode;
   context->getProperty(TailMode.getName(), mode);
 
-  std::string file = "";
-  if (!context->getProperty(FileName.getName(), file)) {
-    throw minifi::Exception(ExceptionType::PROCESSOR_EXCEPTION, "File to Tail is a required property");
-  }
   if (mode == "Multiple file") {
-    // file is a regex
-    std::string base_dir;
-    if (!context->getProperty(BaseDirectory.getName(), base_dir)) {
+    tail_mode_ = Mode::MULTIPLE;
+
+    if (!context->getProperty(BaseDirectory.getName(), base_dir_)) {
       throw minifi::Exception(ExceptionType::PROCESSOR_EXCEPTION, "Base directory is required for multiple tail mode.");

Review comment:
       This is fair, but I would also add a check here to make sure that the directory exists. 
   As far as I remember we have such utility func in fileutils, so not a big change.

##########
File path: extensions/standard-processors/processors/TailFile.cpp
##########
@@ -120,56 +158,42 @@ void TailFile::onSchedule(const std::shared_ptr<core::ProcessContext> &context,
   std::string value;
 
   if (context->getProperty(Delimiter.getName(), value)) {
-    delimiter_ = value;
+    delimiter_ = parseDelimiter(value);
+  }
+
+  if (!context->getProperty(FileName.getName(), file_to_tail_)) {
+    throw minifi::Exception(ExceptionType::PROCESSOR_EXCEPTION, "File to Tail is a required property");

Review comment:
       I would suggest using propertybuilder and make the property required, so this can't happen. 




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

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