fgerlits commented on a change in pull request #1259:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1259#discussion_r806621990
##########
File path: libminifi/include/properties/Properties.h
##########
@@ -29,16 +29,23 @@
#include "core/logging/Logger.h"
#include "utils/ChecksumCalculator.h"
+#include "utils/StringUtils.h"
namespace org {
namespace apache {
namespace nifi {
namespace minifi {
+enum class PropertyChangeLifetime {
+ TRANSIENT, // the changed value will not be committed to disk
+ PERSISTENT // the changed value will be written to the source file
+};
+
class Properties {
struct PropertyValue {
- std::string value;
- bool changed;
+ std::string persisted_value;
+ std::string active_value;
+ bool changed{false}; // persisted_value should be written to disk if
requested
Review comment:
I would rename this to something like `need_to_persist_new_value`, and
then the comment is no longer needed
##########
File path: libminifi/src/core/logging/LoggerConfiguration.cpp
##########
@@ -359,6 +323,57 @@ void LoggerConfiguration::initializeCompression(const
std::lock_guard<std::mutex
}
}
+std::shared_ptr<spdlog::sinks::rotating_file_sink_mt>
LoggerConfiguration::getRotatingFileSink(const std::string& appender_key, const
std::shared_ptr<LoggerProperties>& properties) {
+ static std::map<std::filesystem::path,
std::shared_ptr<spdlog::sinks::rotating_file_sink_mt>> rotating_file_sinks;
+ static std::mutex sink_map_mtx;
+
+ std::string file_name;
+ if (!properties->getString(appender_key + ".file_name", file_name)) {
+ file_name = "minifi-app.log";
+ }
+ std::string directory;
+ if (!properties->getString(appender_key + ".directory", directory)) {
+ // The below part assumes logger_properties->getHome() is existing
+ // Cause minifiHome must be set at MiNiFiMain.cpp?
+ directory = properties->getHome() +
utils::file::FileUtils::get_separator() + "logs";
+ }
+
+ file_name = directory + utils::file::FileUtils::get_separator() + file_name;
+ if (utils::file::FileUtils::create_dir(directory) == -1) {
+ std::cerr << directory << " cannot be created\n";
+ exit(1);
+ }
+
+ int max_files = 3;
+ std::string max_files_str = "";
+ if (properties->getString(appender_key + ".max_files", max_files_str)) {
+ try {
+ max_files = std::stoi(max_files_str);
+ } catch (const std::invalid_argument &) {
+ } catch (const std::out_of_range &) {
+ }
+ }
+
+ int max_file_size = 5_MiB;
+ std::string max_file_size_str = "";
+ if (properties->getString(appender_key + ".max_file_size",
max_file_size_str)) {
+ try {
+ max_file_size = std::stoi(max_file_size_str);
+ } catch (const std::invalid_argument &) {
+ } catch (const std::out_of_range &) {
+ }
+ }
+
+ std::lock_guard<std::mutex> guard(sink_map_mtx);
+ auto it = rotating_file_sinks.find(file_name);
+ if (it != rotating_file_sinks.end()) {
+ return it->second;
+ }
Review comment:
What is the reason for this new caching? It looks a bit weird that we
do it for the rolling appender but not for the other appender types. Is it
possible that some update will not get applied because of a cached old value?
##########
File path: libminifi/src/core/logging/LoggerConfiguration.cpp
##########
@@ -359,6 +323,57 @@ void LoggerConfiguration::initializeCompression(const
std::lock_guard<std::mutex
}
}
+std::shared_ptr<spdlog::sinks::rotating_file_sink_mt>
LoggerConfiguration::getRotatingFileSink(const std::string& appender_key, const
std::shared_ptr<LoggerProperties>& properties) {
+ static std::map<std::filesystem::path,
std::shared_ptr<spdlog::sinks::rotating_file_sink_mt>> rotating_file_sinks;
+ static std::mutex sink_map_mtx;
+
+ std::string file_name;
+ if (!properties->getString(appender_key + ".file_name", file_name)) {
+ file_name = "minifi-app.log";
+ }
+ std::string directory;
+ if (!properties->getString(appender_key + ".directory", directory)) {
+ // The below part assumes logger_properties->getHome() is existing
+ // Cause minifiHome must be set at MiNiFiMain.cpp?
+ directory = properties->getHome() +
utils::file::FileUtils::get_separator() + "logs";
+ }
+
+ file_name = directory + utils::file::FileUtils::get_separator() + file_name;
+ if (utils::file::FileUtils::create_dir(directory) == -1) {
+ std::cerr << directory << " cannot be created\n";
+ exit(1);
+ }
+
+ int max_files = 3;
+ std::string max_files_str = "";
+ if (properties->getString(appender_key + ".max_files", max_files_str)) {
+ try {
+ max_files = std::stoi(max_files_str);
+ } catch (const std::invalid_argument &) {
+ } catch (const std::out_of_range &) {
+ }
+ }
+
+ int max_file_size = 5_MiB;
+ std::string max_file_size_str = "";
+ if (properties->getString(appender_key + ".max_file_size",
max_file_size_str)) {
+ try {
+ max_file_size = std::stoi(max_file_size_str);
+ } catch (const std::invalid_argument &) {
+ } catch (const std::out_of_range &) {
+ }
+ }
+
+ std::lock_guard<std::mutex> guard(sink_map_mtx);
+ auto it = rotating_file_sinks.find(file_name);
+ if (it != rotating_file_sinks.end()) {
+ return it->second;
+ }
Review comment:
Thanks, makes sense. But I don't think the new comment is clear;
something like
```
// According to spdlog docs, if two loggers write to the same file, they
must use the same sink object.
// Note that some logging configuration changes will not take effect until
MiNiFi is restarted.
```
would be 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-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
