fgerlits commented on code in PR #1299: URL: https://github.com/apache/nifi-minifi-cpp/pull/1299#discussion_r858654467
########## libminifi/include/utils/meta/type_list.h: ########## @@ -0,0 +1,34 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once +#include <type_traits> + +namespace org::apache::nifi::minifi::utils::meta { + +template<typename... Types> +struct type_list { + template<typename T> + [[nodiscard]] constexpr static bool contains() noexcept { + return (std::is_same_v<T, Types> || ...); + } + + template<template<typename...> typename Metafn> + using apply = Metafn<Types...>; Review Comment: holy moly -- what does this do? ########## libminifi/include/core/state/Value.h: ########## @@ -497,34 +495,31 @@ static inline std::shared_ptr<Value> createValue(const double &object) { * Purpose: ValueNode is the AST container for a value */ class ValueNode { - public: - ValueNode() - : value_(nullptr) { - } + using supported_types = utils::meta::type_list<int, uint32_t, size_t, int64_t, uint64_t, bool, char*, const char*, double, std::string>; + public: + ValueNode() = default; ValueNode(ValueNode &&vn) = default; ValueNode(const ValueNode &vn) = default; + ValueNode &operator=(const ValueNode &ref) = default; Review Comment: why is there no move assignment? ########## libminifi/include/core/state/nodes/AgentInformation.h: ########## @@ -620,80 +620,38 @@ class AgentManifest : public DeviceInformation { } void setConfigurationReader(std::function<std::optional<std::string>(const std::string&)> configuration_reader) { - configuration_reader_ = configuration_reader; - } - - std::vector<SerializedResponseNode> serialize() { - static std::vector<SerializedResponseNode> serialized; - if (serialized.empty()) { - SerializedResponseNode ident; - - ident.name = "identifier"; - ident.value = AgentBuild::BUILD_IDENTIFIER; - - SerializedResponseNode type; - - type.name = "agentType"; - type.value = "cpp"; - - SerializedResponseNode version; - - version.name = "version"; - version.value = AgentBuild::VERSION; - - SerializedResponseNode buildInfo; - buildInfo.name = "buildInfo"; - - SerializedResponseNode build_version; - build_version.name = "version"; - build_version.value = AgentBuild::VERSION; - - SerializedResponseNode build_rev; - build_rev.name = "revision"; - build_rev.value = AgentBuild::BUILD_REV; - - SerializedResponseNode build_date; - build_date.name = "timestamp"; - build_date.value = (uint64_t) std::stoull(AgentBuild::BUILD_DATE); - - SerializedResponseNode compiler_command; - compiler_command.name = "compiler"; - compiler_command.value = AgentBuild::COMPILER; - - SerializedResponseNode compiler_flags; - compiler_flags.name = "flags"; - compiler_flags.value = AgentBuild::COMPILER_FLAGS; - - buildInfo.children.push_back(compiler_flags); - buildInfo.children.push_back(compiler_command); - - buildInfo.children.push_back(build_version); - buildInfo.children.push_back(build_rev); - buildInfo.children.push_back(build_date); - - Bundles bundles("bundles"); - - serialized.push_back(ident); - serialized.push_back(type); - serialized.push_back(buildInfo); - // serialize the bundle information. - for (auto bundle : bundles.serialize()) { - serialized.push_back(bundle); - } - - SchedulingDefaults defaults("schedulingDefaults"); - - for (auto defaultNode : defaults.serialize()) { - serialized.push_back(defaultNode); - } - - SupportedOperations supported_operations("supportedOperations"); - supported_operations.setStateMonitor(monitor_); - supported_operations.setUpdatePolicyController(update_policy_controller_); - supported_operations.setConfigurationReader(configuration_reader_); - for (const auto& operation : supported_operations.serialize()) { - serialized.push_back(operation); - } + configuration_reader_ = std::move(configuration_reader); + } + + std::vector<SerializedResponseNode> serialize() override { + std::vector<SerializedResponseNode> serialized = { + {.name = "identifier", .value = AgentBuild::BUILD_IDENTIFIER}, + {.name = "agentType", .value = "cpp"}, + {.name = "buildInfo", .children = { + {.name = "flags", .value = AgentBuild::COMPILER_FLAGS}, + {.name = "compiler", .value = AgentBuild::COMPILER}, + {.name = "version", .value = AgentBuild::VERSION}, + {.name = "revision", .value = AgentBuild::BUILD_REV}, + {.name = "timestamp", .value = static_cast<uint64_t>(std::stoull(AgentBuild::BUILD_DATE))} + }} + }; Review Comment: :+1: ########## libminifi/include/core/state/nodes/AgentInformation.h: ########## @@ -721,52 +679,38 @@ class AgentNode : public DeviceInformation, public AgentMonitor, public AgentIde } void setConfigurationReader(std::function<std::optional<std::string>(const std::string&)> configuration_reader) { - configuration_reader_ = configuration_reader; + configuration_reader_ = std::move(configuration_reader); } protected: - std::vector<SerializedResponseNode> serialize() { - std::vector<SerializedResponseNode> serialized; - - SerializedResponseNode ident; - - ident.name = "identifier"; - ident.value = provider_->getAgentIdentifier(); - serialized.push_back(ident); + std::vector<SerializedResponseNode> serialize() override { + std::vector<SerializedResponseNode> serialized = { + {.name = "identifier", .value = provider_->getAgentIdentifier()}, + }; const auto agent_class = provider_->getAgentClass(); if (agent_class) { - SerializedResponseNode agentClass; - agentClass.name = "agentClass"; - agentClass.value = *agent_class; - serialized.push_back(agentClass); + serialized.push_back({.name = "agentClass", .value = *agent_class}); } - SerializedResponseNode agentManifestHash; - agentManifestHash.name = "agentManifestHash"; - agentManifestHash.value = getAgentManifestHash(); - serialized.push_back(agentManifestHash); - + serialized.push_back({.name = "agentManifestHash", .value = getAgentManifestHash()}); return serialized; } std::vector<SerializedResponseNode> getAgentManifest() const { - SerializedResponseNode agentManifest; - agentManifest.name = "agentManifest"; - AgentManifest manifest{"manifest"}; - manifest.setStateMonitor(monitor_); - manifest.setUpdatePolicyController(update_policy_controller_); - manifest.setConfigurationReader(configuration_reader_); - agentManifest.children = manifest.serialize(); - return std::vector<SerializedResponseNode>{ agentManifest }; + if (agent_manifest_cache_) { return std::vector{*agent_manifest_cache_}; } + agent_manifest_cache_ = {.name = "agentManifest", .children = [this] { + AgentManifest manifest{"manifest"}; + manifest.setStateMonitor(monitor_); + manifest.setUpdatePolicyController(update_policy_controller_); + manifest.setConfigurationReader(configuration_reader_); + return manifest.serialize(); + }()}; + return std::vector{ *agent_manifest_cache_ }; } std::string getAgentManifestHash() { - if (!agentManifestHash_.has_value()) { - agentManifestHash_ = hashResponseNodes(getAgentManifest()); - } - - return *agentManifestHash_; + return hashResponseNodes(getAgentManifest()); Review Comment: It seems wasteful to recompute the hash every time, even if `agent_manifest_cache_` hasn't changed. Could we move the computation of the hash to inside `getAgentManifest()`? ########## libminifi/src/c2/C2Client.cpp: ########## @@ -68,7 +71,23 @@ void C2Client::initialize(core::controller::ControllerServiceProvider *controlle logger_->log_info("Agent class is not predefined"); } - configuration_->setFallbackAgentIdentifier(getControllerUUID().to_string()); + // Set a persistent fallback agent id. This is needed so that the C2 server can identify the same agent after a restart, even if nifi.c2.agent.identifier is not specified. Review Comment: Do we really want to create this file even if `nifi.c2.agent.identifier` is specified? I would find that confusing as a user. Also, why don't we put this ID into minifi.properties, as the value of `nifi.c2.agent.identifier`? We already write stuff there, eg. `nifi.c2.flow.id` and `nifi.c2.flow.url`. ########## libminifi/include/utils/FifoExecutor.h: ########## @@ -49,8 +49,9 @@ class WorkerThread final { /** * A worker that executes arbitrary functions with no parameters asynchronously on an internal thread, returning a future to the result. + * This is different from the Worker in ThreadPool.h in that it's only capable of running tasks, and any scheduling is left for the user. Review Comment: Since the name changed, the comment can be shortened, too: ```suggestion * Executes arbitrary functions with no parameters asynchronously on an internal thread, returning a future to the result. ``` -- 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]
