From 08c8bd4aee3377b6bc4ce0000522013f6365a8a1 Mon Sep 17 00:00:00 2001 From: sale Date: Tue, 20 Dec 2016 22:23:54 +0000 Subject: [PATCH 1/4] Initial configuration memgraph which needs review Summary: MG configuration commit for review Test Plan: manual Reviewers: buda Subscribers: buda Differential Revision: https://memgraph.phacility.com/D24 --- include/config/config.hpp | 1 + include/utils/config/config.hpp | 104 ++++++++++++++++++-------------- src/config/config.cpp | 2 +- src/memgraph_bolt.cpp | 3 + 4 files changed, 64 insertions(+), 46 deletions(-) diff --git a/include/config/config.hpp b/include/config/config.hpp index fb15307f3..837340ae9 100644 --- a/include/config/config.hpp +++ b/include/config/config.hpp @@ -31,6 +31,7 @@ inline size_t to_int(std::string &&s) { return stoull(s); } // code uses this define for key access // _KEY_ is value from all possible keys that are listed above +#define CONFIG_INIT() config::Config::instance().initialize() #define CONFIG(_KEY_) config::Config::instance()[_KEY_] #define CONFIG_INTEGER(_KEY_) config::to_int(CONFIG(_KEY_)) diff --git a/include/utils/config/config.hpp b/include/utils/config/config.hpp index 7fcec7182..a23a2e795 100644 --- a/include/utils/config/config.hpp +++ b/include/utils/config/config.hpp @@ -1,63 +1,77 @@ #pragma once -#include -#include -#include #include +#include +#include +#include +#include + +#include +#include +#include // TODO: isolate from caller (caller shouldn't know that his dependency is) // yaml-cpp +#include "utils/command_line/arguments.hpp" #include "yaml-cpp/yaml.h" -namespace config -{ - +namespace config { + template -class Config -{ -private: - YAML::Node _config; +class Config { + private: + using KeyValueMap = std::map; - Config() - { - // config places: priority - // 1. default system | (/etc/name/config) - // 2. default user | (/home/user/.name/config) - // 3. ENV var | (PATH) - // 4. program argument \ / (PATH) - - if (const char* env_path = std::getenv(Definition::env_config_key)) - { - _config = YAML::LoadFile(env_path); - // TODO: error handling - } - else - { - _config = YAML::LoadFile(Definition::default_file_path); - // TODO: error handling - } + KeyValueMap dict; - // TODO: - // * default system - // * default user - // * program argument + void load_configuration(std::string path) { + try { + YAML::Node node = YAML::LoadFile(path); + + for (YAML::const_iterator it = node.begin(); it != node.end(); ++it) { + dict[it->first.as()] = it->second.as(); + } + } catch (std::exception ex) { + // configuration doesn't exist or invalid!!! } + } -public: - static Config& instance() - { - static Config config; - return config; - } + Config() { + // config places: priority + // 1. default system | (/etc/memgraph/config) + // 2. default user | (/home/user/.memgraph/config) + // 3. ENV var | (PATH) + // 4. program argument \ / (PATH) - void register_program_arguments() - { - } + // default system configuration + load_configuration(Definition::default_file_path); - std::string operator[](const char* key) - { - return _config[key].template as(); + // default user configuration + // fetches user configuration folder + std::string homedir; + if ((homedir = getenv("HOME")) == "") { + homedir = getpwuid(getuid())->pw_dir; } + homedir += "/.memgraph/config.yaml"; + load_configuration(homedir); + + // environment variable configuratoin + if (const char* env_path = std::getenv(Definition::env_config_key)) + load_configuration(env_path); + + // TODO add program arguments here + // Define how will we pass program args and which ones are we using and how. + // ProgramArguments::instance().get_arg(); + } + + public: + static Config& instance() { + static Config config; + return config; + } + + std::string operator[](const char* key) { + return dict[key]; + } }; - } diff --git a/src/config/config.cpp b/src/config/config.cpp index bf8f4700f..ef47d2445 100644 --- a/src/config/config.cpp +++ b/src/config/config.cpp @@ -4,6 +4,6 @@ namespace config { const char *MemgraphConfig::env_config_key = "MEMGRAPH_CONFIG"; -const char *MemgraphConfig::default_file_path = "config.yaml"; +const char *MemgraphConfig::default_file_path = "/etc/memgraph/config.yaml"; } diff --git a/src/memgraph_bolt.cpp b/src/memgraph_bolt.cpp index 1c0585068..4de296343 100644 --- a/src/memgraph_bolt.cpp +++ b/src/memgraph_bolt.cpp @@ -9,6 +9,7 @@ #include "logging/default.hpp" #include "logging/streams/stdout.hpp" +#include "utils/config/config.hpp" #include "utils/signals/handler.hpp" #include "utils/stacktrace.hpp" #include "utils/terminate_handler.hpp" @@ -57,6 +58,8 @@ int main(void) { exit(1); }); + // CONFIG call + io::Socket socket; try { From 013f69b1ca1a793391305d16f797b4125ea09a5f Mon Sep 17 00:00:00 2001 From: sale Date: Tue, 20 Dec 2016 22:29:33 +0000 Subject: [PATCH 2/4] removed unused and obsolete macro --- include/config/config.hpp | 1 - 1 file changed, 1 deletion(-) diff --git a/include/config/config.hpp b/include/config/config.hpp index 837340ae9..fb15307f3 100644 --- a/include/config/config.hpp +++ b/include/config/config.hpp @@ -31,7 +31,6 @@ inline size_t to_int(std::string &&s) { return stoull(s); } // code uses this define for key access // _KEY_ is value from all possible keys that are listed above -#define CONFIG_INIT() config::Config::instance().initialize() #define CONFIG(_KEY_) config::Config::instance()[_KEY_] #define CONFIG_INTEGER(_KEY_) config::to_int(CONFIG(_KEY_)) From 02f332c34fd39fb62579ee4add6210661934cf2b Mon Sep 17 00:00:00 2001 From: sale Date: Wed, 21 Dec 2016 17:48:38 +0000 Subject: [PATCH 3/4] Update on Configuration and CppCheck refactor --- include/config/config.hpp | 6 ++++++ include/query/language/cypher/ast/tree.hpp | 5 +---- .../query/language/cypher/debug/tree_print.hpp | 5 +++-- include/threading/pool.hpp | 2 +- include/utils/config/config.hpp | 17 +++++++++++------ src/config/config.cpp | 9 +++++++++ src/dbms/dbms.cpp | 3 --- src/memgraph_bolt.cpp | 4 ++-- src/speedy/http/http_error.hpp | 5 ++--- src/speedy/http/httpserver.hpp | 2 +- src/speedy/http/httpserver.inl | 4 ++-- src/speedy/http/response.hpp | 2 +- src/speedy/http/response.inl | 4 ++-- src/speedy/r3.hpp | 6 ++---- src/test/lockfree_list.cpp | 2 +- src/utils/string/transform.cpp | 1 + 16 files changed, 45 insertions(+), 32 deletions(-) diff --git a/include/config/config.hpp b/include/config/config.hpp index fb15307f3..1b9e64dd7 100644 --- a/include/config/config.hpp +++ b/include/config/config.hpp @@ -2,6 +2,9 @@ #include "utils/config/config.hpp" +#include +#include + namespace config { @@ -15,6 +18,7 @@ class MemgraphConfig public: static const char *env_config_key; static const char *default_file_path; + static std::set arguments; }; // -- all possible Memgraph's keys -- @@ -31,6 +35,8 @@ inline size_t to_int(std::string &&s) { return stoull(s); } // code uses this define for key access // _KEY_ is value from all possible keys that are listed above +#define CONFIG_REGISTER_ARGS(ARGC, ARGV) \ + config::Config::instance().register_args(ARGC, ARGV); #define CONFIG(_KEY_) config::Config::instance()[_KEY_] #define CONFIG_INTEGER(_KEY_) config::to_int(CONFIG(_KEY_)) diff --git a/include/query/language/cypher/ast/tree.hpp b/include/query/language/cypher/ast/tree.hpp index 9b8b7ca2a..c19d53ad6 100644 --- a/include/query/language/cypher/ast/tree.hpp +++ b/include/query/language/cypher/ast/tree.hpp @@ -13,12 +13,9 @@ public: Ast() = default; Ast(const Ast&) = delete; - Ast(Ast&& other) + Ast(Ast&& other) : root(other.root), items(std::move(other.items)) { - this->root = other.root; other.root = nullptr; - - this->items = std::move(other.items); } Ast& operator=(Ast&& other) diff --git a/include/query/language/cypher/debug/tree_print.hpp b/include/query/language/cypher/debug/tree_print.hpp index f08c53590..b46742abb 100644 --- a/include/query/language/cypher/debug/tree_print.hpp +++ b/include/query/language/cypher/debug/tree_print.hpp @@ -24,7 +24,7 @@ public: class Entry { public: - Entry(Printer &printer) : printer(printer), valid(true) + explicit Entry(Printer &printer) : printer(printer), valid(true) { printer.level++; @@ -77,7 +77,8 @@ public: size_t level = 0; }; - PrintVisitor(std::ostream &stream) : printer(stream, "Printing AST") {} + explicit PrintVisitor(std::ostream &stream) + : printer(stream, "Printing AST") {} void visit(ast::Start &start) override { diff --git a/include/threading/pool.hpp b/include/threading/pool.hpp index 3a7c399bb..021ca8916 100644 --- a/include/threading/pool.hpp +++ b/include/threading/pool.hpp @@ -14,7 +14,7 @@ class Pool : Lockable public: using sptr = std::shared_ptr; - Pool(size_t n = std::thread::hardware_concurrency()) : alive(true) + explicit Pool(size_t n = std::thread::hardware_concurrency()) : alive(true) { threads.reserve(n); diff --git a/include/utils/config/config.hpp b/include/utils/config/config.hpp index a23a2e795..3ff7d708a 100644 --- a/include/utils/config/config.hpp +++ b/include/utils/config/config.hpp @@ -48,8 +48,8 @@ class Config { // default user configuration // fetches user configuration folder - std::string homedir; - if ((homedir = getenv("HOME")) == "") { + std::string homedir = std::getenv("HOME"); + if ((homedir == "") { homedir = getpwuid(getuid())->pw_dir; } homedir += "/.memgraph/config.yaml"; @@ -58,10 +58,6 @@ class Config { // environment variable configuratoin if (const char* env_path = std::getenv(Definition::env_config_key)) load_configuration(env_path); - - // TODO add program arguments here - // Define how will we pass program args and which ones are we using and how. - // ProgramArguments::instance().get_arg(); } public: @@ -70,6 +66,15 @@ class Config { return config; } + void register_args(int argc, char** argv) { + REGISTER_ARGS(argc, argv); + + for (const auto& argument : Definition::arguments) { + dict[argument] = GET_ARG("-" + argument, dict[argument]).get_string(); + } + + } + std::string operator[](const char* key) { return dict[key]; } diff --git a/src/config/config.cpp b/src/config/config.cpp index ef47d2445..708e5e2e7 100644 --- a/src/config/config.cpp +++ b/src/config/config.cpp @@ -6,4 +6,13 @@ namespace config const char *MemgraphConfig::env_config_key = "MEMGRAPH_CONFIG"; const char *MemgraphConfig::default_file_path = "/etc/memgraph/config.yaml"; +// configuration for arguments which can be passed threw command line +// TODO add support for shortened names +// Example: +// --cleaning_cycle_sec or -ccs, etc. +std::set MemgraphConfig::arguments = { + "cleaning_cycle_sec", + "snapshot_cycle_sec", +}; + } diff --git a/src/dbms/dbms.cpp b/src/dbms/dbms.cpp index 4d4af07a5..442dfcb13 100644 --- a/src/dbms/dbms.cpp +++ b/src/dbms/dbms.cpp @@ -20,9 +20,6 @@ Db &Dbms::active(const std::string &name) // create db if it doesn't exist auto it = acc.find(name); if (it == acc.end()) { - - // It doesn't exist. - Snapshoter &snap = snapshoter; it = acc.emplace(name, std::forward_as_tuple(name), std::forward_as_tuple(name)) .first; diff --git a/src/memgraph_bolt.cpp b/src/memgraph_bolt.cpp index 4de296343..8e721af47 100644 --- a/src/memgraph_bolt.cpp +++ b/src/memgraph_bolt.cpp @@ -58,13 +58,13 @@ int main(void) { exit(1); }); - // CONFIG call + // TODO CONFIG call io::Socket socket; try { socket = io::Socket::bind(interface, port); - } catch (io::NetworkError e) { + } catch (const io::NetworkError& e) { logger.error("Cannot bind to socket on {} at {}", interface, port); logger.error("{}", e.what()); diff --git a/src/speedy/http/http_error.hpp b/src/speedy/http/http_error.hpp index 642cb7d85..7b6dae578 100644 --- a/src/speedy/http/http_error.hpp +++ b/src/speedy/http/http_error.hpp @@ -1,5 +1,4 @@ -#ifndef MEMGRAPH_SERVER_HTTP_HTTP_ERROR_HPP -#define MEMGRAPH_SERVER_HTTP_HTTP_ERROR_HPP +#pragma once #include #include @@ -10,7 +9,7 @@ namespace http class HttpError : public std::runtime_error { public: - HttpError(const std::string& message) + explicit HttpError(const std::string& message) : std::runtime_error(message) {} }; diff --git a/src/speedy/http/httpserver.hpp b/src/speedy/http/httpserver.hpp index f72db7730..c70ccc278 100644 --- a/src/speedy/http/httpserver.hpp +++ b/src/speedy/http/httpserver.hpp @@ -22,7 +22,7 @@ class HttpServer public: using request_cb_t = std::function; - HttpServer(uv::UvLoop& loop); + explicit HttpServer(uv::UvLoop& loop); void listen(const Ipv4& ip, request_cb_t callback); diff --git a/src/speedy/http/httpserver.inl b/src/speedy/http/httpserver.inl index ea4df5f0d..4bdba2e60 100644 --- a/src/speedy/http/httpserver.inl +++ b/src/speedy/http/httpserver.inl @@ -9,8 +9,8 @@ namespace http static BlockAllocator<65536> buffer_allocator; template -HttpServer::HttpServer(uv::UvLoop& loop) - : loop(loop), stream(loop) {} +HttpServer::HttpServer(uv::UvLoop& l) + : loop(l), stream(l) {} template void HttpServer::listen(const Ipv4& ip, request_cb_t callback) diff --git a/src/speedy/http/response.hpp b/src/speedy/http/response.hpp index 2fd233d77..705b4c4da 100644 --- a/src/speedy/http/response.hpp +++ b/src/speedy/http/response.hpp @@ -21,7 +21,7 @@ class Response using response_t = Response; public: - Response(connection_t& connection); + explicit Response(connection_t& connection); void send(const std::string& body); void send(Status code, const std::string& body); diff --git a/src/speedy/http/response.inl b/src/speedy/http/response.inl index 4179b813d..5383cd05c 100644 --- a/src/speedy/http/response.inl +++ b/src/speedy/http/response.inl @@ -12,8 +12,8 @@ namespace http static BlockAllocator write_req_allocator; template -Response::Response(connection_t& connection) - : status(Status::Ok), connection(connection), buffer() {} +Response::Response(connection_t& conn) + : status(Status::Ok), connection(conn), buffer() {} template void Response::send(Status status, const std::string& body) diff --git a/src/speedy/r3.hpp b/src/speedy/r3.hpp index 74d545583..eba379e62 100644 --- a/src/speedy/r3.hpp +++ b/src/speedy/r3.hpp @@ -92,7 +92,7 @@ public: r3::route* route; }; - R3(size_t capacity) + explicit R3(size_t capacity) { root = r3::r3_tree_create(capacity); } @@ -104,10 +104,8 @@ public: R3(R3&) = delete; - R3(R3&& other) + R3(R3&& other) : routes(std::move(other.routes), root(other.root) { - this->routes = std::move(other.routes); - this->root = other.root; other.root = nullptr; } diff --git a/src/test/lockfree_list.cpp b/src/test/lockfree_list.cpp index af1f5eaa4..e16326f03 100644 --- a/src/test/lockfree_list.cpp +++ b/src/test/lockfree_list.cpp @@ -10,7 +10,7 @@ using namespace std; template class ListNode { public: - ListNode(T value) : value(value) {} + explicit ListNode(T value) : value(value) {} std::atomic value; std::atomic*> prev; diff --git a/src/utils/string/transform.cpp b/src/utils/string/transform.cpp index 20a06f2d7..f443d91ce 100644 --- a/src/utils/string/transform.cpp +++ b/src/utils/string/transform.cpp @@ -3,6 +3,7 @@ namespace utils { +// TODO CPPCheck -> function never used void str_tolower(std::string& s) { // en_US.utf8 localization From 1c1cf1ad882ea69f2543fd5cb622064478b49bf3 Mon Sep 17 00:00:00 2001 From: sale Date: Wed, 28 Dec 2016 11:56:53 +0000 Subject: [PATCH 4/4] Review changes fixed --- include/utils/config/config.hpp | 4 ++-- src/memgraph_bolt.cpp | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/include/utils/config/config.hpp b/include/utils/config/config.hpp index 3ff7d708a..b36976cd3 100644 --- a/include/utils/config/config.hpp +++ b/include/utils/config/config.hpp @@ -49,7 +49,7 @@ class Config { // default user configuration // fetches user configuration folder std::string homedir = std::getenv("HOME"); - if ((homedir == "") { + if ((homedir == "")) { homedir = getpwuid(getuid())->pw_dir; } homedir += "/.memgraph/config.yaml"; @@ -70,7 +70,7 @@ class Config { REGISTER_ARGS(argc, argv); for (const auto& argument : Definition::arguments) { - dict[argument] = GET_ARG("-" + argument, dict[argument]).get_string(); + dict[argument] = GET_ARG("--" + argument, dict[argument]).get_string(); } } diff --git a/src/memgraph_bolt.cpp b/src/memgraph_bolt.cpp index 8e721af47..a26df5d3c 100644 --- a/src/memgraph_bolt.cpp +++ b/src/memgraph_bolt.cpp @@ -26,7 +26,7 @@ void throw_and_stacktace(std::string message) { logger.info(stacktrace.dump()); } -int main(void) { +int main(int argc, char** argv) { // TODO figure out what is the relationship between this and signals // that are configured below std::set_terminate(&terminate_handler); @@ -58,7 +58,8 @@ int main(void) { exit(1); }); - // TODO CONFIG call + + CONFIG_REGISTER_ARGS(argc, argv); io::Socket socket;