From 59035d9892831400041e529a3bc69e8b297903a3 Mon Sep 17 00:00:00 2001 From: Jin Hai Date: Fri, 17 Apr 2020 21:05:17 +0800 Subject: [PATCH] #1454 Improve code quality (#1948) * Improve code quality Signed-off-by: jinhai * Improve code quality Signed-off-by: jinhai * Improve code quality Signed-off-by: jinhai * Fix lint Signed-off-by: JinHai-CN * Fix compiling error Signed-off-by: JinHai-CN * Fix compiling error Signed-off-by: JinHai-CN * Fix compiling error Signed-off-by: JinHai-CN * Fix issue Signed-off-by: JinHai-CN --- CHANGELOG.md | 2 +- core/cmake/ThirdPartyPackages.cmake | 14 +--- .../default/DefaultVectorIndexFormat.cpp | 2 - core/src/config/Config.cpp | 77 ++++++++++++------- core/src/config/ConfigNode.cpp | 5 +- core/src/db/meta/MySQLConnectionPool.cpp | 7 +- core/src/db/meta/MySQLConnectionPool.h | 20 ++--- core/src/db/meta/MySQLMetaImpl.cpp | 2 +- .../index/cmake/ThirdPartyPackagesCore.cmake | 1 - core/src/utils/ValidationUtil.cpp | 5 ++ core/src/utils/ValidationUtil.h | 3 + sdk/cmake/ThirdPartyPackages.cmake | 2 - 12 files changed, 73 insertions(+), 67 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5832eb1a37..e36394e286 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -284,7 +284,7 @@ Please mark all change in change log and use the issue from GitHub ## Improvement - \#255 Add ivfsq8 test report detailed version - \#260 C++ SDK README -- \#266 Rpc request source code refactor +- \#266 RPC request source code refactor - \#274 Logger the time cost during preloading data - \#275 Rename C++ SDK IndexType - \#284 Change C++ SDK to shared library diff --git a/core/cmake/ThirdPartyPackages.cmake b/core/cmake/ThirdPartyPackages.cmake index 29b4c6e4b4..c1d59cfeb7 100644 --- a/core/cmake/ThirdPartyPackages.cmake +++ b/core/cmake/ThirdPartyPackages.cmake @@ -240,14 +240,12 @@ else () "https://github.com/google/googletest/archive/release-${GTEST_VERSION}.tar.gz" "https://gitee.com/quicksilver/googletest/repository/archive/release-${GTEST_VERSION}.zip") endif () -set(GTEST_MD5 "2e6fbeb6a91310a16efe181886c59596") if (DEFINED ENV{MILVUS_MYSQLPP_URL}) set(MYSQLPP_SOURCE_URL "$ENV{MILVUS_MYSQLPP_URL}") else () set(MYSQLPP_SOURCE_URL "https://tangentsoft.com/mysqlpp/releases/mysql++-${MYSQLPP_VERSION}.tar.gz") endif () -set(MYSQLPP_MD5 "cda38b5ecc0117de91f7c42292dd1e79") if (DEFINED ENV{MILVUS_PROMETHEUS_URL}) set(PROMETHEUS_SOURCE_URL "$ENV{PROMETHEUS_OPENBLAS_URL}") @@ -262,7 +260,6 @@ else () set(SQLITE_SOURCE_URL "https://www.sqlite.org/2019/sqlite-autoconf-${SQLITE_VERSION}.tar.gz") endif () -set(SQLITE_MD5 "3c68eb400f8354605736cd55400e1572") if (DEFINED ENV{MILVUS_SQLITE_ORM_URL}) set(SQLITE_ORM_SOURCE_URLS "$ENV{MILVUS_SQLITE_ORM_URL}") @@ -271,7 +268,6 @@ else () "https://github.com/fnc12/sqlite_orm/archive/${SQLITE_ORM_VERSION}.zip" "https://gitee.com/quicksilver/sqlite_orm/repository/archive/${SQLITE_ORM_VERSION}.zip") endif () -set(SQLITE_ORM_MD5 "ba9a405a8a1421c093aa8ce988ff8598") if (DEFINED ENV{MILVUS_YAMLCPP_URL}) set(YAMLCPP_SOURCE_URL "$ENV{MILVUS_YAMLCPP_URL}") @@ -279,7 +275,6 @@ else () set(YAMLCPP_SOURCE_URL "https://github.com/jbeder/yaml-cpp/archive/yaml-cpp-${YAMLCPP_VERSION}.tar.gz" "https://gitee.com/quicksilver/yaml-cpp/repository/archive/yaml-cpp-${YAMLCPP_VERSION}.zip") endif () -set(YAMLCPP_MD5 "5b943e9af0060d0811148b037449ef82") if (DEFINED ENV{MILVUS_LIBUNWIND_URL}) set(LIBUNWIND_SOURCE_URL "$ENV{MILVUS_LIBUNWIND_URL}") @@ -287,7 +282,6 @@ else () set(LIBUNWIND_SOURCE_URL "https://github.com/libunwind/libunwind/releases/download/v${LIBUNWIND_VERSION}/libunwind-${LIBUNWIND_VERSION}.tar.gz") endif () -set(LIBUNWIND_MD5 "a04f69d66d8e16f8bf3ab72a69112cd6") if (DEFINED ENV{MILVUS_GPERFTOOLS_URL}) set(GPERFTOOLS_SOURCE_URL "$ENV{MILVUS_GPERFTOOLS_URL}") @@ -295,17 +289,16 @@ else () set(GPERFTOOLS_SOURCE_URL "https://github.com/gperftools/gperftools/releases/download/gperftools-${GPERFTOOLS_VERSION}/gperftools-${GPERFTOOLS_VERSION}.tar.gz") endif () -set(GPERFTOOLS_MD5 "c6a852a817e9160c79bdb2d3101b4601") if (DEFINED ENV{MILVUS_GRPC_URL}) set(GRPC_SOURCE_URL "$ENV{MILVUS_GRPC_URL}") else () set(GRPC_SOURCE_URL "https://github.com/milvus-io/grpc-milvus/archive/${GRPC_VERSION}.zip" - "https://github.com/youny626/grpc-milvus/archive/${GRPC_VERSION}.zip" - "https://gitee.com/quicksilver/grpc-milvus/repository/archive/${GRPC_VERSION}.zip") + #"https://github.com/youny626/grpc-milvus/archive/${GRPC_VERSION}.zip" + #"https://gitee.com/quicksilver/grpc-milvus/repository/archive/${GRPC_VERSION}.zip" + ) endif () -set(GRPC_MD5 "0362ba219f59432c530070b5f5c3df73") if (DEFINED ENV{MILVUS_ZLIB_URL}) set(ZLIB_SOURCE_URL "$ENV{MILVUS_ZLIB_URL}") @@ -313,7 +306,6 @@ else () set(ZLIB_SOURCE_URL "https://github.com/madler/zlib/archive/${ZLIB_VERSION}.tar.gz" "https://gitee.com/quicksilver/zlib/repository/archive/${ZLIB_VERSION}.zip") endif () -set(ZLIB_MD5 "0095d2d2d1f3442ce1318336637b695f") if (DEFINED ENV{MILVUS_OPENTRACING_URL}) set(OPENTRACING_SOURCE_URL "$ENV{MILVUS_OPENTRACING_URL}") diff --git a/core/src/codecs/default/DefaultVectorIndexFormat.cpp b/core/src/codecs/default/DefaultVectorIndexFormat.cpp index 84eed66316..0deb5ac252 100644 --- a/core/src/codecs/default/DefaultVectorIndexFormat.cpp +++ b/core/src/codecs/default/DefaultVectorIndexFormat.cpp @@ -121,8 +121,6 @@ DefaultVectorIndexFormat::write(const storage::FSHandlerPtr& fs_ptr, const std:: const segment::VectorIndexPtr& vector_index) { const std::lock_guard lock(mutex_); - std::string dir_path = fs_ptr->operation_ptr_->GetDirectory(); - milvus::TimeRecorder recorder("write_index"); knowhere::VecIndexPtr index = vector_index->GetVectorIndex(); diff --git a/core/src/config/Config.cpp b/core/src/config/Config.cpp index 964b55d017..a3636ef71b 100644 --- a/core/src/config/Config.cpp +++ b/core/src/config/Config.cpp @@ -39,6 +39,9 @@ namespace server { constexpr int64_t GB = 1UL << 30; +constexpr int32_t PORT_NUMBER_MIN = 1024; +constexpr int32_t PORT_NUMBER_MAX = 65535; + static const std::unordered_map milvus_config_version_map( {{"0.6.0", "0.1"}, {"0.7.0", "0.2"}, {"0.7.1", "0.2"}, {"0.8.0", "0.3"}}); @@ -698,11 +701,15 @@ Config::CheckServerConfigPort(const std::string& value) { std::string msg = "Invalid server port: " + value + ". Possible reason: server_config.port is not a number."; return Status(SERVER_INVALID_ARGUMENT, msg); } else { - int32_t port = std::stoi(value); - if (!(port > 1024 && port < 65535)) { - std::string msg = "Invalid server port: " + value + - ". Possible reason: server_config.port is not in range (1024, 65535)."; - return Status(SERVER_INVALID_ARGUMENT, msg); + try { + int32_t port = std::stoi(value); + if (!(port > PORT_NUMBER_MIN && port < PORT_NUMBER_MAX)) { + std::string msg = "Invalid server port: " + value + + ". Possible reason: server_config.port is not in range (1024, 65535)."; + return Status(SERVER_INVALID_ARGUMENT, msg); + } + } catch (...) { + return Status(SERVER_INVALID_ARGUMENT, "Invalid server_config.port: " + value); } } return Status::OK(); @@ -732,9 +739,7 @@ Config::CheckServerConfigTimeZone(const std::string& value) { if (value.substr(0, 3) != "UTC") { return Status(SERVER_INVALID_ARGUMENT, "Invalid server_config.time_zone: " + value); } else { - try { - stoi(value.substr(3)); - } catch (...) { + if (!ValidationUtil::IsNumber(value.substr(4))) { return Status(SERVER_INVALID_ARGUMENT, "Invalid server_config.time_zone: " + value); } } @@ -749,11 +754,15 @@ Config::CheckServerConfigWebPort(const std::string& value) { "Invalid web server port: " + value + ". Possible reason: server_config.web_port is not a number."; return Status(SERVER_INVALID_ARGUMENT, msg); } else { - int32_t port = std::stoi(value); - if (!(port > 1024 && port < 65535)) { - std::string msg = "Invalid web server port: " + value + - ". Possible reason: server_config.web_port is not in range [1025, 65534]."; - return Status(SERVER_INVALID_ARGUMENT, msg); + try { + int32_t port = std::stoi(value); + if (!(port > PORT_NUMBER_MIN && port < PORT_NUMBER_MAX)) { + std::string msg = "Invalid web server port: " + value + + ". Possible reason: server_config.web_port is not in range (1024, 65535)."; + return Status(SERVER_INVALID_ARGUMENT, msg); + } + } catch (...) { + return Status(SERVER_INVALID_ARGUMENT, "Invalid server_config.web_port: " + value); } } return Status::OK(); @@ -915,11 +924,15 @@ Config::CheckStorageConfigS3Port(const std::string& value) { std::string msg = "Invalid s3 port: " + value + ". Possible reason: storage_config.s3_port is not a number."; return Status(SERVER_INVALID_ARGUMENT, msg); } else { - int32_t port = std::stoi(value); - if (!(port > 1024 && port < 65535)) { - std::string msg = "Invalid s3 port: " + value + - ". Possible reason: storage_config.s3_port is not in range (1024, 65535)."; - return Status(SERVER_INVALID_ARGUMENT, msg); + try { + int32_t port = std::stoi(value); + if (!(port > PORT_NUMBER_MIN && port < PORT_NUMBER_MAX)) { + std::string msg = "Invalid s3 port: " + value + + ". Possible reason: storage_config.s3_port is not in range (1024, 65535)."; + return Status(SERVER_INVALID_ARGUMENT, msg); + } + } catch (...) { + return Status(SERVER_INVALID_ARGUMENT, "Invalid storage_config.s3_port: " + value); } } return Status::OK(); @@ -978,11 +991,15 @@ Config::CheckMetricConfigPort(const std::string& value) { std::string msg = "Invalid metric port: " + value + ". Possible reason: metric_config.port is not a number."; return Status(SERVER_INVALID_ARGUMENT, msg); } else { - int32_t port = std::stoi(value); - if (!(port > 1024 && port < 65535)) { - std::string msg = "Invalid metric port: " + value + - ". Possible reason: metric_config.port is not in range (1024, 65535)."; - return Status(SERVER_INVALID_ARGUMENT, msg); + try { + int32_t port = std::stoi(value); + if (!(port > PORT_NUMBER_MIN && port < PORT_NUMBER_MAX)) { + std::string msg = "Invalid metric port: " + value + + ". Possible reason: metric_config.port is not in range (1024, 65535)."; + return Status(SERVER_INVALID_ARGUMENT, msg); + } + } catch (...) { + return Status(SERVER_INVALID_ARGUMENT, "Invalid metric_config.port: " + value); } } return Status::OK(); @@ -1224,11 +1241,15 @@ CheckGpuResource(const std::string& value) { } if (s.compare(0, 3, "gpu") == 0) { - int32_t gpu_index = std::stoi(s.substr(3)); - if (!ValidationUtil::ValidateGpuIndex(gpu_index).ok()) { - std::string msg = "Invalid gpu resource: " + value + - ". Possible reason: gpu_resource_config does not match with the hardware."; - return Status(SERVER_INVALID_ARGUMENT, msg); + try { + int32_t gpu_index = std::stoi(s.substr(3)); + if (!ValidationUtil::ValidateGpuIndex(gpu_index).ok()) { + std::string msg = "Invalid gpu resource: " + value + + ". Possible reason: gpu_resource_config does not match with the hardware."; + return Status(SERVER_INVALID_ARGUMENT, msg); + } + } catch (...) { + return Status(SERVER_INVALID_ARGUMENT, "Invalid gpu_resource_config: " + value); } } diff --git a/core/src/config/ConfigNode.cpp b/core/src/config/ConfigNode.cpp index 7125334cbf..deeccb954c 100644 --- a/core/src/config/ConfigNode.cpp +++ b/core/src/config/ConfigNode.cpp @@ -142,9 +142,8 @@ ConfigNode::GetChild(const std::string& type_name) { void ConfigNode::GetChildren(ConfigNodeArr& arr) const { arr.clear(); - for (auto ref : children_) { - arr.push_back(ref.second); - } + arr.reserve(children_.size()); + transform(children_.begin(), children_.end(), back_inserter(arr), [](auto& ref) { return ref.second; }); } const std::map& diff --git a/core/src/db/meta/MySQLConnectionPool.cpp b/core/src/db/meta/MySQLConnectionPool.cpp index c28da4b097..e036f52c2b 100644 --- a/core/src/db/meta/MySQLConnectionPool.cpp +++ b/core/src/db/meta/MySQLConnectionPool.cpp @@ -51,11 +51,6 @@ MySQLConnectionPool::release(const mysqlpp::Connection* pc) { // max_idle_time_ = max_idle; // } -std::string -MySQLConnectionPool::getDB() { - return db_; -} - // Superclass overrides mysqlpp::Connection* MySQLConnectionPool::create() { @@ -66,7 +61,7 @@ MySQLConnectionPool::create() { // creation. auto conn = new mysqlpp::Connection(); conn->set_option(new mysqlpp::ReconnectOption(true)); - conn->connect(db_.empty() ? 0 : db_.c_str(), server_.empty() ? 0 : server_.c_str(), + conn->connect(db_name_.empty() ? 0 : db_name_.c_str(), server_.empty() ? 0 : server_.c_str(), user_.empty() ? 0 : user_.c_str(), password_.empty() ? 0 : password_.c_str(), port_); return conn; } catch (const mysqlpp::ConnectionFailed& er) { diff --git a/core/src/db/meta/MySQLConnectionPool.h b/core/src/db/meta/MySQLConnectionPool.h index 02378126af..bab0ddabcd 100644 --- a/core/src/db/meta/MySQLConnectionPool.h +++ b/core/src/db/meta/MySQLConnectionPool.h @@ -26,14 +26,12 @@ class MySQLConnectionPool : public mysqlpp::ConnectionPool { // The object's only constructor MySQLConnectionPool(const std::string& dbName, const std::string& userName, const std::string& passWord, const std::string& serverIp, int port = 0, int maxPoolSize = 8) - : db_(dbName), + : db_name_(dbName), user_(userName), password_(passWord), server_(serverIp), port_(port), max_pool_size_(maxPoolSize) { - conns_in_use_ = 0; - max_idle_time_ = 10; // 10 seconds } // The destructor. We _must_ call ConnectionPool::clear() here, @@ -49,12 +47,10 @@ class MySQLConnectionPool : public mysqlpp::ConnectionPool { void release(const mysqlpp::Connection* pc) override; - // int getConnectionsInUse(); - // - // void set_max_idle_time(int max_idle); - - std::string - getDB(); + const std::string& + db_name() const { + return db_name_; + } protected: // Superclass overrides @@ -69,15 +65,15 @@ class MySQLConnectionPool : public mysqlpp::ConnectionPool { private: // Number of connections currently in use - std::atomic conns_in_use_; + std::atomic conns_in_use_ = 0; // Our connection parameters - std::string db_, user_, password_, server_; + std::string db_name_, user_, password_, server_; int port_; int max_pool_size_; - unsigned int max_idle_time_; + unsigned int max_idle_time_ = 0; // 10 seconds }; } // namespace meta diff --git a/core/src/db/meta/MySQLMetaImpl.cpp b/core/src/db/meta/MySQLMetaImpl.cpp index cfb4af3502..3878b2b650 100644 --- a/core/src/db/meta/MySQLMetaImpl.cpp +++ b/core/src/db/meta/MySQLMetaImpl.cpp @@ -2130,7 +2130,7 @@ MySQLMetaImpl::CleanUpShadowFiles() { mysqlpp::Query statement = connectionPtr->query(); statement << "SELECT table_name" << " FROM information_schema.tables" - << " WHERE table_schema = " << mysqlpp::quote << mysql_connection_pool_->getDB() + << " WHERE table_schema = " << mysqlpp::quote << mysql_connection_pool_->db_name() << " AND table_name = " << mysqlpp::quote << META_TABLEFILES << ";"; LOG_ENGINE_DEBUG_ << "CleanUp: " << statement.str(); diff --git a/core/src/index/cmake/ThirdPartyPackagesCore.cmake b/core/src/index/cmake/ThirdPartyPackagesCore.cmake index 3026e5d1e0..96de5b8702 100644 --- a/core/src/index/cmake/ThirdPartyPackagesCore.cmake +++ b/core/src/index/cmake/ThirdPartyPackagesCore.cmake @@ -216,7 +216,6 @@ else () set(GTEST_SOURCE_URL "https://github.com/google/googletest/archive/release-${GTEST_VERSION}.tar.gz") endif () -set(GTEST_MD5 "2e6fbeb6a91310a16efe181886c59596") # ---------------------------------------------------------------------- # ARROW diff --git a/core/src/utils/ValidationUtil.cpp b/core/src/utils/ValidationUtil.cpp index 755f6f069d..521c86c4ce 100644 --- a/core/src/utils/ValidationUtil.cpp +++ b/core/src/utils/ValidationUtil.cpp @@ -651,5 +651,10 @@ ValidationUtil::ValidateStoragePath(const std::string& path) { return std::regex_match(path, regex) ? Status::OK() : Status(SERVER_INVALID_ARGUMENT, "Invalid file path"); } +bool +ValidationUtil::IsNumber(const std::string& s) { + return !s.empty() && std::all_of(s.begin(), s.end(), ::isdigit); +} + } // namespace server } // namespace milvus diff --git a/core/src/utils/ValidationUtil.h b/core/src/utils/ValidationUtil.h index 0c30dd1457..825cec977a 100644 --- a/core/src/utils/ValidationUtil.h +++ b/core/src/utils/ValidationUtil.h @@ -92,6 +92,9 @@ class ValidationUtil { static Status ValidateStoragePath(const std::string& path); + + static bool + IsNumber(const std::string& s); }; } // namespace server diff --git a/sdk/cmake/ThirdPartyPackages.cmake b/sdk/cmake/ThirdPartyPackages.cmake index ae67fb24b6..099cc4812a 100644 --- a/sdk/cmake/ThirdPartyPackages.cmake +++ b/sdk/cmake/ThirdPartyPackages.cmake @@ -187,14 +187,12 @@ else () set(GRPC_SOURCE_URL "https://github.com/youny626/grpc-milvus/archive/master.zip") endif () -set(GRPC_MD5 "0362ba219f59432c530070b5f5c3df73") if (DEFINED ENV{MILVUS_ZLIB_URL}) set(ZLIB_SOURCE_URL "$ENV{MILVUS_ZLIB_URL}") else () set(ZLIB_SOURCE_URL "https://github.com/madler/zlib/archive/v1.2.11.tar.gz") endif () -set(ZLIB_MD5 "0095d2d2d1f3442ce1318336637b695f") # ---------------------------------------------------------------------- # GRPC