refactor: remove the unnecessary zookeeper_session_mgr class (apache#2356)

The `zookeeper_session_mgr::get_session(...)` can be made static, so it can be extracted
and implemented as a separate global function. Setting the ZooKeeper C client’s log file
handle can be done together with the log level, ensuring it is executed only once globally
and before any `zhandle_t` object is created. Thus entire `zookeeper_session_mgr` class
could be removed.

In addition, error info would be printed if the the ZooKeeper C client's log file failed to be
opened. Also introduce mutex to support concurrent operations to obtain ZooKeeper sessions
by multiple threads.

Author: empiredan

.licenserc.yaml

@@ -668,8 +668,6 @@ header:
     - 'src/zookeeper/zookeeper_error.h'
     - 'src/zookeeper/zookeeper_session.cpp'
     - 'src/zookeeper/zookeeper_session.h'
-    - 'src/zookeeper/zookeeper_session_mgr.cpp'
-    - 'src/zookeeper/zookeeper_session_mgr.h'
     # Apache License, Version 2.0, Copyright 2018 Google LLC
     - 'src/gutil/test/map_traits_test.cpp'
     - 'src/gutil/test/map_util_test.h'

src/meta/meta_state_service_zookeeper.cpp

@@ -41,7 +41,6 @@
 #include "utils/utils.h"
 #include "zookeeper/zookeeper_error.h"
 #include "zookeeper/zookeeper_session.h"
-#include "zookeeper/zookeeper_session_mgr.h"
 
 DSN_DECLARE_int32(timeout_ms);
 
@@ -160,8 +159,7 @@ meta_state_service_zookeeper::~meta_state_service_zookeeper()
 
 error_code meta_state_service_zookeeper::initialize(const std::vector<std::string> &)
 {
-    _session =
-        zookeeper_session_mgr::instance().get_session(service_app::current_service_app_info());
+    _session = get_zookeeper_session(service_app::current_service_app_info());
     _zoo_state = _session->attach(this,
                                   std::bind(&meta_state_service_zookeeper::on_zoo_session_evt,
                                             ref_this(this),

src/zookeeper/distributed_lock_service_zookeeper.cpp

@@ -39,7 +39,6 @@
 #include "utils/strings.h"
 #include "zookeeper/lock_struct.h"
 #include "zookeeper/lock_types.h"
-#include "zookeeper/zookeeper_session_mgr.h"
 #include "zookeeper_error.h"
 #include "zookeeper_session.h"
 
@@ -94,8 +93,7 @@ error_code distributed_lock_service_zookeeper::initialize(const std::vector<std:
     }
     const char *lock_root = args[0].c_str();
 
-    _session =
-        zookeeper_session_mgr::instance().get_session(service_app::current_service_app_info());
+    _session = get_zookeeper_session(service_app::current_service_app_info());
     _zoo_state = _session->attach(this,
                                   std::bind(&distributed_lock_service_zookeeper::on_zoo_session_evt,
                                             lock_srv_ptr(this),

src/zookeeper/zookeeper_session.cpp

@@ -31,7 +31,9 @@
 #include <zookeeper/zookeeper.h>
 #include <algorithm>
 #include <cerrno>
+#include <cstdio>
 #include <cstdlib>
+#include <mutex>
 #include <utility>
 
 #include "rpc/rpc_address.h"
@@ -42,6 +44,7 @@
 #include "utils/flags.h"
 #include "utils/fmt_logging.h"
 #include "utils/safe_strerror_posix.h"
+#include "utils/singleton_store.h"
 #include "utils/strings.h"
 #include "zookeeper/proto.h"
 #include "zookeeper/zookeeper.jute.h"
@@ -70,6 +73,7 @@ DSN_DEFINE_int32(zookeeper,
                  timeout_ms,
                  30000,
                  "The timeout of accessing ZooKeeper, in milliseconds");
+DSN_DEFINE_string(zookeeper, logfile, "zoo.log", "The path of the log file for ZooKeeper C client");
 DSN_DEFINE_string(zookeeper, zoo_log_level, "ZOO_LOG_LEVEL_INFO", "ZooKeeper log level");
 DSN_DEFINE_string(zookeeper, hosts_list, "", "ZooKeeper hosts list");
 DSN_DEFINE_string(zookeeper, sasl_service_name, "zookeeper", "");
@@ -119,8 +123,7 @@ DSN_DEFINE_group_validator(consistency_between_configurations, [](std::string &m
     return true;
 });
 
-namespace dsn {
-namespace dist {
+namespace dsn::dist {
 
 zookeeper_session::zoo_atomic_packet::zoo_atomic_packet(unsigned int size)
 {
@@ -274,9 +277,32 @@ bool is_password_file_plaintext()
 
 zhandle_t *create_zookeeper_handle(watcher_fn watcher, void *context)
 {
-    const auto zoo_log_level = enum_from_string(FLAGS_zoo_log_level, INVALID_ZOO_LOG_LEVEL);
-    CHECK(zoo_log_level != INVALID_ZOO_LOG_LEVEL, "Invalid zoo log level: {}", FLAGS_zoo_log_level);
-    zoo_set_debug_level(zoo_log_level);
+    static std::once_flag flag;
+    std::call_once(flag, []() {
+        FILE *zoo_log_file = std::fopen(FLAGS_logfile, "a");
+        if (zoo_log_file == nullptr) {
+            LOG_ERROR("failed to open the log file for ZooKeeper C Client, use stderr instead: "
+                      "path = {}, error = {}",
+                      FLAGS_logfile,
+                      utils::safe_strerror(errno));
+            zoo_set_log_stream(stderr);
+        } else {
+            // The file handle pointed to by `zoo_log_file` will never be released because:
+            // 1. Each meta server process holds only one log file handle.
+            // 2. It cannot be guaranteed that the handle will no longer be used in some
+            // background thread of ZooKeeper C Client after being released even by atexit(),
+            // since zhandle_t objects are not closed in the end. Once `fclose` is called on
+            // the log file handle, any subsequent write operations on it would result in
+            // undefined behavior.
+            zoo_set_log_stream(zoo_log_file);
+        }
+
+        const auto zoo_log_level = enum_from_string(FLAGS_zoo_log_level, INVALID_ZOO_LOG_LEVEL);
+        CHECK(zoo_log_level != INVALID_ZOO_LOG_LEVEL,
+              "Invalid zoo log level: {}",
+              FLAGS_zoo_log_level);
+        zoo_set_debug_level(zoo_log_level);
+    });
 
     // SASL auth is enabled iff FLAGS_sasl_mechanisms_type is non-empty.
     if (dsn::utils::is_empty(FLAGS_sasl_mechanisms_type)) {
@@ -565,5 +591,21 @@ void zookeeper_session::global_void_completion(int rc, const void *data)
     release_ref(op_ctx);
 }
 
-} // namespace dist
-} // namespace dsn
+zookeeper_session *get_zookeeper_session(const service_app_info &info)
+{
+    static std::mutex mtx;
+    auto &store = utils::singleton_store<int, zookeeper_session *>::instance();
+
+    zookeeper_session *session{nullptr};
+    {
+        std::lock_guard<std::mutex> lock(mtx);
+        if (!store.get(info.entity_id, session)) {
+            session = new zookeeper_session(info);
+            store.put(info.entity_id, session);
+        }
+    }
+
+    return session;
+}
+
+} // namespace dsn::dist

src/zookeeper/zookeeper_session.h

@@ -205,6 +205,11 @@ class zookeeper_session
     DISALLOW_MOVE_AND_ASSIGN(zookeeper_session);
 };
 
+// Obtain a ZooKeeper session. Each service node has at most one ZooKeeper session; if none
+// exists for the service app, one will be created. The obtained ZooKeeper session can be
+// shared across multiple threads in one service node. This function is thread-safe.
+zookeeper_session *get_zookeeper_session(const service_app_info &info);
+
 } // namespace dsn::dist
 
 USER_DEFINED_STRUCTURE_FORMATTER(::dsn::dist::zookeeper_session);