szaszm commented on a change in pull request #1253:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1253#discussion_r806085046
##########
File path: encrypt-config/ConfigFile.cpp
##########
@@ -23,11 +23,10 @@
#include <optional>
#include "utils/StringUtils.h"
+#include "properties/Configuration.h"
namespace {
-constexpr std::array<const char*, 2>
DEFAULT_SENSITIVE_PROPERTIES{"nifi.security.client.pass.phrase",
-
"nifi.rest.api.password"};
-constexpr const char* ADDITIONAL_SENSITIVE_PROPS_PROPERTY_NAME =
"nifi.sensitive.props.additional.keys";
+
Review comment:
Please remove this empty namespace
##########
File path: encrypt-config/tests/ConfigFileEncryptorTests.cpp
##########
@@ -76,13 +77,13 @@ TEST_CASE("ConfigFileEncryptor can encrypt the sensitive
properties", "[encrypt-
SECTION("default properties") {
ConfigFile test_file{std::ifstream{"resources/minifi.properties"}};
- std::string original_password =
test_file.getValue("nifi.rest.api.password").value();
+ std::string original_password =
test_file.getValue(org::apache::nifi::minifi::Configuration::nifi_rest_api_password).value();
Review comment:
Consider either moving the tests into the minifi namespace or using an
alias. There are already aliases on line 28-30, but I don't think it would be
too difficult to wrap the file.
##########
File path: libminifi/include/properties/Configuration.h
##########
@@ -99,6 +139,30 @@ class Configuration : public Properties {
static constexpr const char *minifi_disk_space_watchdog_interval =
"minifi.disk.space.watchdog.interval";
static constexpr const char *minifi_disk_space_watchdog_stop_threshold =
"minifi.disk.space.watchdog.stop.threshold";
static constexpr const char *minifi_disk_space_watchdog_restart_threshold =
"minifi.disk.space.watchdog.restart.threshold";
+
+ // JNI options
+ static constexpr const char *nifi_framework_dir = "nifi.framework.dir";
+ static constexpr const char *nifi_jvm_options = "nifi.jvm.options";
+ static constexpr const char *nifi_nar_directory = "nifi.nar.directory";
+ static constexpr const char *nifi_nar_deploy_directory =
"nifi.nar.deploy.directory";
+
+ // Log options
+ static constexpr const char *nifi_log_spdlog_pattern =
"nifi.log.spdlog.pattern";
+ static constexpr const char *nifi_log_spdlog_shorten_names =
"nifi.log.spdlog.shorten_names";
+ static constexpr const char *nifi_log_appender_rolling =
"nifi.log.appender.rolling";
+ static constexpr const char *nifi_log_appender_rolling_directory =
"nifi.log.appender.rolling.directory";
+ static constexpr const char *nifi_log_appender_rolling_file_name =
"nifi.log.appender.rolling.file_name";
+ static constexpr const char *nifi_log_appender_rolling_max_files =
"nifi.log.appender.rolling.max_files";
+ static constexpr const char *nifi_log_appender_rolling_max_file_size =
"nifi.log.appender.rolling.max_file_size";
+ static constexpr const char *nifi_log_appender_stdout =
"nifi.log.appender.stdout";
+ static constexpr const char *nifi_log_appender_stderr =
"nifi.log.appender.stderr";
+ static constexpr const char *nifi_log_appender_null =
"nifi.log.appender.null";
+ static constexpr const char *nifi_log_appender_syslog =
"nifi.log.appender.syslog";
+ static constexpr const char *nifi_log_logger_root = "nifi.log.logger.root";
+ static constexpr const char *nifi_log_compression_cached_log_max_size =
"nifi.log.compression.cached.log.max.size";
+ static constexpr const char *nifi_log_compression_compressed_log_max_size =
"nifi.log.compression.compressed.log.max.size";
+
+ MINIFIAPI static const std::vector<core::ConfigurationProperty>
CONFIGURATION_PROPERTIES;
Review comment:
Instead of introducing a vector, we should change the type of the
constants instead to carry the necessary information. I'm thinking of this or
similar:
```
static constexpr core::ConfigurationProperty
nifi_flow_configuration_encrypt{"nifi.flow.configuration.encrypt",
core::validator<bool>{}};
```
Where `core::ConfigurationProperty` is a class template, with a `constexpr`
constructor, using CTAD here, and is able to statically dispatch to the
appropriate validation function, because why wouldn't we if all the information
is available at compile-time.
##########
File path: libminifi/include/core/Property.h
##########
@@ -467,6 +468,16 @@ class ConstrainedProperty : public
std::enable_shared_from_this<ConstrainedPrope
friend class PropertyBuilder;
};
+struct ConfigurationProperty {
+ explicit ConfigurationProperty(std::string_view name_, const
gsl::not_null<std::shared_ptr<PropertyValidator>>& validator_ =
StandardValidators::get().VALID_VALIDATOR)
+ : name(name_),
+ validator(validator_) {
+ }
+
+ std::string_view name;
+ gsl::not_null<std::shared_ptr<PropertyValidator>> validator;
Review comment:
Consider using a reference of a `not_null` raw (observer) pointer instead
##########
File path: libminifi/src/Configure.cpp
##########
@@ -68,15 +69,15 @@ bool Configure::isEncrypted(const std::string& key) const {
std::optional<std::string> Configure::getAgentClass() const {
std::string agent_class;
- if (get("nifi.c2.agent.class", "c2.agent.class", agent_class) &&
!agent_class.empty()) {
+ if (get(minifi::Configuration::nifi_c2_agent_class, "c2.agent.class",
agent_class) && !agent_class.empty()) {
return agent_class;
}
return {};
}
std::string Configure::getAgentIdentifier() const {
std::string agent_id;
- if (!get("nifi.c2.agent.identifier", "c2.agent.identifier", agent_id) ||
agent_id.empty()) {
+ if (!get(minifi::Configuration::nifi_c2_agent_identifier,
"c2.agent.identifier", agent_id) || agent_id.empty()) {
Review comment:
The minifi prefix is not necessary inside the minifi namespace
##########
File path: libminifi/test/unit/SocketTests.cpp
##########
@@ -221,7 +222,7 @@ TEST_CASE("TestTLSContextCreation", "[TestSocket8]") {
*/
TEST_CASE("TestTLSContextCreation2", "[TestSocket9]") {
std::shared_ptr<minifi::Configure> configure =
std::make_shared<minifi::Configure>();
- configure->set("nifi.remote.input.secure", "false");
+
configure->set(org::apache::nifi::minifi::Configuration::nifi_remote_input_secure,
"false");
Review comment:
There is already a minifi namespace alias, use it!
##########
File path: libminifi/src/Configure.cpp
##########
@@ -68,15 +69,15 @@ bool Configure::isEncrypted(const std::string& key) const {
std::optional<std::string> Configure::getAgentClass() const {
std::string agent_class;
- if (get("nifi.c2.agent.class", "c2.agent.class", agent_class) &&
!agent_class.empty()) {
+ if (get(minifi::Configuration::nifi_c2_agent_class, "c2.agent.class",
agent_class) && !agent_class.empty()) {
Review comment:
The minifi prefix is not necessary inside the minifi namespace
##########
File path: libminifi/src/c2/C2Agent.cpp
##########
@@ -164,7 +164,7 @@ void C2Agent::configure(const std::shared_ptr<Configure>
&configure, bool reconf
protocol_.load()->update(configure);
}
- if (configure->get("nifi.c2.agent.heartbeat.period",
"c2.agent.heartbeat.period", heartbeat_period)) {
+ if (configure->get(minifi::Configuration::nifi_c2_agent_heartbeat_period,
"c2.agent.heartbeat.period", heartbeat_period)) {
Review comment:
The minifi prefix is not necessary inside the minifi namespace
##########
File path: libminifi/src/c2/C2Agent.cpp
##########
@@ -138,7 +138,7 @@ void C2Agent::configure(const std::shared_ptr<Configure>
&configure, bool reconf
std::string clazz, heartbeat_period, device;
if (!reconfigure) {
- if (!configure->get("nifi.c2.agent.protocol.class",
"c2.agent.protocol.class", clazz)) {
+ if (!configure->get(minifi::Configuration::nifi_c2_agent_protocol_class,
"c2.agent.protocol.class", clazz)) {
Review comment:
The minifi prefix is not necessary inside the minifi namespace
##########
File path: extensions/http-curl/protocols/RESTSender.cpp
##########
@@ -43,9 +44,9 @@ void
RESTSender::initialize(core::controller::ControllerServiceProvider* control
// base URL when one is not specified.
if (nullptr != configure) {
std::string update_str, ssl_context_service_str;
- configure->get("nifi.c2.rest.url", "c2.rest.url", rest_uri_);
- configure->get("nifi.c2.rest.url.ack", "c2.rest.url.ack", ack_uri_);
- if (configure->get("nifi.c2.rest.ssl.context.service",
"c2.rest.ssl.context.service", ssl_context_service_str)) {
+ configure->get(minifi::Configuration::nifi_c2_rest_url, "c2.rest.url",
rest_uri_);
+ configure->get(minifi::Configuration::nifi_c2_rest_url_ack,
"c2.rest.url.ack", ack_uri_);
+ if
(configure->get(minifi::Configuration::nifi_c2_rest_ssl_context_service,
"c2.rest.ssl.context.service", ssl_context_service_str)) {
Review comment:
The minifi prefix is not necessary inside the minifi namespace
##########
File path: extensions/http-curl/protocols/RESTSender.cpp
##########
@@ -74,8 +75,8 @@ C2Payload RESTSender::consumePayload(const C2Payload
&payload, Direction directi
void RESTSender::update(const std::shared_ptr<Configure> &configure) {
std::string url;
- configure->get("nifi.c2.rest.url", "c2.rest.url", url);
- configure->get("nifi.c2.rest.url.ack", "c2.rest.url.ack", url);
+ configure->get(minifi::Configuration::nifi_c2_rest_url, "c2.rest.url", url);
+ configure->get(minifi::Configuration::nifi_c2_rest_url_ack,
"c2.rest.url.ack", url);
Review comment:
The minifi prefix is not necessary inside the minifi namespace
##########
File path: libminifi/src/c2/C2Agent.cpp
##########
@@ -181,7 +181,7 @@ void C2Agent::configure(const std::shared_ptr<Configure>
&configure, bool reconf
}
std::string heartbeat_reporters;
- if (configure->get("nifi.c2.agent.heartbeat.reporter.classes",
"c2.agent.heartbeat.reporter.classes", heartbeat_reporters)) {
+ if
(configure->get(minifi::Configuration::nifi_c2_agent_heartbeat_reporter_classes,
"c2.agent.heartbeat.reporter.classes", heartbeat_reporters)) {
Review comment:
The minifi prefix is not necessary inside the minifi namespace
I'm going to stop adding individual comments on these now, but there are
plenty of them left. Please remove the unnecessary namespace qualifiers.
##########
File path: libminifi/src/c2/C2Agent.cpp
##########
@@ -334,44 +334,7 @@ struct C2DebugBundleError : public C2TransferError {
void C2Agent::handle_c2_server_response(const C2ContentResponse &resp) {
switch (resp.op.value()) {
case Operation::CLEAR:
- // we've been told to clear something
- if (resp.name == "connection") {
- for (const auto& connection : resp.operation_arguments) {
- logger_->log_debug("Clearing connection %s",
connection.second.to_string());
- update_sink_->clearConnection(connection.second.to_string());
- }
- C2Payload response(Operation::ACKNOWLEDGE, resp.ident, true);
- enqueue_c2_response(std::move(response));
- } else if (resp.name == "repositories") {
- update_sink_->drainRepositories();
- C2Payload response(Operation::ACKNOWLEDGE, resp.ident, true);
- enqueue_c2_response(std::move(response));
- } else if (resp.name == "corecomponentstate") {
- // TODO(bakaid): untested
- std::vector<std::shared_ptr<state::StateController>> components =
update_sink_->getComponents(resp.name);
- auto state_manager_provider =
core::ProcessContext::getStateManagerProvider(logger_, controller_,
configuration_);
- if (state_manager_provider != nullptr) {
- for (auto &component : components) {
- logger_->log_debug("Clearing state for component %s",
component->getComponentName());
- auto state_manager =
state_manager_provider->getCoreComponentStateManager(component->getComponentUUID());
- if (state_manager != nullptr) {
- component->stop();
- state_manager->clear();
- state_manager->persist();
- component->start();
- } else {
- logger_->log_warn("Failed to get StateManager for component %s",
component->getComponentUUID().to_string());
- }
- }
- } else {
- logger_->log_error("Failed to get StateManagerProvider");
- }
- C2Payload response(Operation::ACKNOWLEDGE, resp.ident, true);
- enqueue_c2_response(std::move(response));
- } else {
- logger_->log_debug("Clearing unknown %s", resp.name);
- }
-
+ handle_clear(resp);
Review comment:
This change is in another PR, I think it shouldn't be replicated here.
--
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]
