#1454 Improve code quality (#1948)

* Improve code quality Signed-off-by: jinhai <hai.jin@zilliz.com> * Improve code quality Signed-off-by: jinhai <hai.jin@zilliz.com> * Improve code quality Signed-off-by: jinhai <hai.jin@zilliz.com> * Fix lint Signed-off-by: JinHai-CN <hai.jin@zilliz.com> * Fix compiling error Signed-off-by: JinHai-CN <hai.jin@zilliz.com> * Fix compiling error Signed-off-by: JinHai-CN <hai.jin@zilliz.com> * Fix compiling error Signed-off-by: JinHai-CN <hai.jin@zilliz.com> * Fix issue Signed-off-by: JinHai-CN <hai.jin@zilliz.com>
2020-04-17 21:05:17 +08:00 · 2020-04-17 21:05:17 +08:00 · 59035d9892
parent 6b106cbef1
commit 59035d9892
12 changed files with 73 additions and 67 deletions
--- 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
--- 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}")
--- 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<std::mutex> lock(mutex_);

-    std::string dir_path = fs_ptr->operation_ptr_->GetDirectory();
-
    milvus::TimeRecorder recorder("write_index");

    knowhere::VecIndexPtr index = vector_index->GetVectorIndex();
--- 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<std::string, std::string> 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);
        }
    }

--- 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<std::string, ConfigNode>&
--- 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) {
--- 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<int> conns_in_use_;
+    std::atomic<int> 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
--- 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();
--- 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
--- 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
--- 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
--- 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