szaszm commented on code in PR #1543:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1543#discussion_r1150080892
##########
minifi_main/MiNiFiMain.cpp:
##########
@@ -263,6 +263,7 @@ int main(int argc, char **argv) {
configure->setHome(minifiHome);
configure->loadConfigureFile(DEFAULT_NIFI_PROPERTIES_FILE);
+ configure->commitChanges();
Review Comment:
Can we skip this commit, and still have the changed/mapped properties appear
as part of the manifest?
It feels wrong to write the config right after reading it, and we will do
the mapping of the values anyway while reading. I think persisting them in a
batch with the first normal persist is enough, and we can rely on in-memory
mapping integers to milliseconds.
##########
libminifi/src/properties/Properties.cpp:
##########
@@ -87,13 +128,17 @@ void Properties::loadConfigureFile(const
std::filesystem::path& configuration_fi
return;
}
properties_.clear();
+ dirty_ = false;
for (const auto& line : PropertiesFile{file}) {
+ auto key = line.getKey();
auto persisted_value = line.getValue();
auto value =
utils::StringUtils::replaceEnvironmentVariables(persisted_value);
- properties_[line.getKey()] = {persisted_value, value, false};
+ bool need_to_persist_new_value = false;
+ formatConfigurationProperty(key, persisted_value, value,
need_to_persist_new_value);
+ dirty_ |= need_to_persist_new_value;
Review Comment:
This operator does bitwise OR, not logical OR. It happens to work out fine,
due to the memory representation of booleans, but I'd prefer using logical
operators with booleans.
##########
libminifi/src/properties/Properties.cpp:
##########
@@ -62,6 +63,46 @@ int Properties::getInt(const std::string &key, int
default_value) const {
return it != properties_.end() ? std::stoi(it->second.active_value) :
default_value;
}
+namespace {
+void ensureTimePeriodValidatedPropertyHasExplicitUnit(const
core::PropertyValidator* const validator, std::string& persisted_value,
std::string& value, bool& need_to_persist_new_value) {
+ if (validator != core::StandardValidators::get().TIME_PERIOD_VALIDATOR.get())
+ return;
+ if (value.empty() || !std::all_of(value.begin(), value.end(), ::isdigit))
+ return;
+
+ value += " ms";
+ persisted_value = value;
+ need_to_persist_new_value = true;
+}
+
+bool integerValidatedProperty(const core::PropertyValidator* const validator) {
+ return validator == core::StandardValidators::get().INTEGER_VALIDATOR.get()
+ || validator ==
core::StandardValidators::get().UNSIGNED_INT_VALIDATOR.get()
+ || validator == core::StandardValidators::get().LONG_VALIDATOR.get()
+ || validator ==
core::StandardValidators::get().UNSIGNED_LONG_VALIDATOR.get();
+}
+
+void ensureIntegerValidatedPropertyHasNoUnit(const core::PropertyValidator*
const validator, std::string& persisted_value, std::string& value, bool&
need_to_persist_new_value) {
+ if (!integerValidatedProperty(validator))
+ return;
+
+ if (auto parsed_time =
utils::timeutils::StringToDuration<std::chrono::milliseconds>(value)) {
+ value = fmt::format("{}", parsed_time->count());
+ persisted_value = value;
+ need_to_persist_new_value = true;
+ }
+}
+
+void formatConfigurationProperty(std::string_view key, std::string&
persisted_value, std::string& value, bool& need_to_persist_new_value) {
+ auto configuration_property =
Configuration::CONFIGURATION_PROPERTIES.find(key);
+ if (configuration_property == Configuration::CONFIGURATION_PROPERTIES.end())
+ return;
+
+
ensureTimePeriodValidatedPropertyHasExplicitUnit(configuration_property->second,
persisted_value, value, need_to_persist_new_value);
+ ensureIntegerValidatedPropertyHasNoUnit(configuration_property->second,
persisted_value, value, need_to_persist_new_value);
+}
+} // namespace
+
Review Comment:
Could you extend the comment above `loadConfigureFile` to specify the kind
of mapping that's happening here during load, and the motivation? Just to avoid
confusing future readers of the code.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
