Modify default session timeout (#3357)

* Add gflag validator for --session_idle_timeout_secs

* Add tests

* Fix pytest

* Add logs for invalid value handling

conf/nebula-graphd.conf.default

@@ -51,10 +51,11 @@
 --reuse_port=false
 # Backlog of the listen socket, adjust this together with net.core.somaxconn
 --listen_backlog=1024
-# Seconds before the idle connections are closed, 0 for never closed
---client_idle_timeout_secs=0
-# Seconds before the idle sessions are expired, 0 for no expiration
---session_idle_timeout_secs=0
+# The number of seconds Nebula service waits before closing the idle connections
+--client_idle_timeout_secs=28800
+# The number of seconds before idle sessions expire
+# The range should be in [1, 604800]
+--session_idle_timeout_secs=28800
 # The number of threads to accept incoming connections
 --num_accept_threads=1
 # The number of networking IO threads, 0 for # of CPU cores

conf/nebula-graphd.conf.production

@@ -49,10 +49,11 @@
 --reuse_port=false
 # Backlog of the listen socket, adjust this together with net.core.somaxconn
 --listen_backlog=1024
-# Seconds before the idle connections are closed, 0 for never closed
---client_idle_timeout_secs=0
-# Seconds before the idle sessions are expired, 0 for no expiration
---session_idle_timeout_secs=60000
+# The number of seconds Nebula service waits before closing the idle connections
+--client_idle_timeout_secs=28800
+# The number of seconds before idle sessions expire
+# The range should be in [1, 604800]
+--session_idle_timeout_secs=28800
 # The number of threads to accept incoming connections
 --num_accept_threads=1
 # The number of networking IO threads, 0 for # of CPU cores

src/graph/executor/admin/ConfigExecutor.cpp

@@ -63,9 +63,30 @@ folly::Future<Status> SetConfigExecutor::execute() {
   SCOPED_TIMER(&execTime_);
 
   auto *scNode = asNode<SetConfig>(node());
+  auto module = scNode->getModule();
+  auto name = scNode->getName();
+  auto value = scNode->getValue();
+
+  // This is a workaround for gflag value validation
+  // Currently, only --session_idle_timeout_secs has a gflag validtor
+  if (module == meta::cpp2::ConfigModule::GRAPH) {
+    // Update local cache before sending request
+    // Gflag value will be checked in SetCommandLineOption() if the flag validtor is registed
+    auto valueStr = meta::GflagsManager::ValueToGflagString(value);
+    if (value.isMap() && value.getMap().kvs.empty()) {
+      // Be compatible with previous configuration
+      valueStr = "{}";
+    }
+    // Check result
+    auto setRes = gflags::SetCommandLineOption(name.c_str(), valueStr.c_str());
+    if (setRes.empty()) {
+      return Status::Error("Invalid value %s for gflag --%s", valueStr.c_str(), name.c_str());
+    }
+  }
+
   return qctx()
       ->getMetaClient()
-      ->setConfig(scNode->getModule(), scNode->getName(), scNode->getValue())
+      ->setConfig(module, name, value)
       .via(runner())
       .thenValue([scNode](StatusOr<bool> resp) {
         if (!resp.ok()) {

src/graph/service/GraphFlags.cpp

@@ -9,15 +9,13 @@
 
 DEFINE_int32(port, 3699, "Nebula Graph daemon's listen port");
 DEFINE_int32(client_idle_timeout_secs,
-             0,
-             "Seconds before we close the idle connections, 0 for infinite");
-DEFINE_int32(session_idle_timeout_secs,
-             0,
-             "Seconds before we expire the idle sessions, 0 for infinite");
+             28800,
+             "The number of seconds Nebula service waits before closing the idle connections");
+DEFINE_int32(session_idle_timeout_secs, 28800, "The number of seconds before idle sessions expire");
 DEFINE_int32(session_reclaim_interval_secs, 10, "Period we try to reclaim expired sessions");
 DEFINE_int32(num_netio_threads,
              0,
-             "Number of networking threads, 0 for number of physical CPU cores");
+             "The number of networking threads, 0 for number of physical CPU cores");
 DEFINE_int32(num_accept_threads, 1, "Number of threads to accept incoming connections");
 DEFINE_int32(num_worker_threads, 0, "Number of threads to execute user queries");
 DEFINE_bool(reuse_port, true, "Whether to turn on the SO_REUSEPORT option");
@@ -71,3 +69,16 @@ DEFINE_string(client_white_list,
               "A white list for different client versions, separate with colon.");
 
 DEFINE_int32(num_rows_to_check_memory, 1024, "number rows to check memory");
+
+// Sanity-checking Flag Values
+static bool ValidateSessIdleTimeout(const char* flagname, int32_t value) {
+  // The max timeout is 604800 seconds(a week)
+  if (value > 0 && value < 604801)  // value is ok
+    return true;
+
+  FLOG_WARN("Invalid value for --%s: %d, the timeout should be an integer between 1 and 604800\n",
+            flagname,
+            (int)value);
+  return false;
+}
+DEFINE_validator(session_idle_timeout_secs, &ValidateSessIdleTimeout);

src/graph/session/GraphSessionManager.cpp

@@ -151,7 +151,10 @@ void GraphSessionManager::threadFunc() {
 // TODO(dutor) Now we do a brute-force scanning, of course we could make it more
 // efficient.
 void GraphSessionManager::reclaimExpiredSessions() {
+  DCHECK_GT(FLAGS_session_idle_timeout_secs, 0);
   if (FLAGS_session_idle_timeout_secs == 0) {
+    LOG(ERROR) << "Program should not reach here, session_idle_timeout_secs should be an integer "
+                  "between 1 and 604800";
     return;
   }
 

tests/admin/test_configs.py

@@ -36,6 +36,12 @@ def test_configs(self):
         resp = self.client.execute('UPDATE CONFIGS storage:v={}'.format(3))
         self.check_resp_succeeded(resp)
 
+        # update flag to an invalid value, expected to fail
+        resp = self.client.execute('UPDATE CONFIGS graph:session_idle_timeout_secs={}'.format(0))
+        self.check_resp_failed(resp)
+        resp = self.client.execute('UPDATE CONFIGS graph:session_idle_timeout_secs={}'.format(999999))
+        self.check_resp_failed(resp)
+
         # get
         resp = self.client.execute('GET CONFIGS meta:v')
         self.check_resp_failed(resp)
@@ -66,7 +72,7 @@ def test_configs(self):
             ['GRAPH', 'accept_partial_success', 'bool', 'MUTABLE', False],
             ['GRAPH', 'system_memory_high_watermark_ratio', 'float', 'MUTABLE', 0.95],
             ['GRAPH', 'num_rows_to_check_memory', 'int', 'MUTABLE', 4],
-            ['GRAPH', 'session_idle_timeout_secs', 'int', 'MUTABLE', 0],
+            ['GRAPH', 'session_idle_timeout_secs', 'int', 'MUTABLE', 28800],
             ['GRAPH', 'session_reclaim_interval_secs', 'int', 'MUTABLE', 2],
             ['GRAPH', 'max_allowed_connections', 'int', 'MUTABLE', 9223372036854775807],
             ['GRAPH', 'disable_octal_escape_char', 'bool', 'MUTABLE', False],
@@ -101,7 +107,7 @@ def test_configs(self):
                                    max_write_buffer_number="4"}
                                    ''')
         self.check_resp_succeeded(resp)
-
+        
         # get result
         resp = self.client.execute('GET CONFIGS storage:rocksdb_column_family_options')
         self.check_resp_succeeded(resp)

tests/job/test_session.py

@@ -127,7 +127,7 @@ def test_sessions(self):
             time.sleep(3)
         resp = self.execute('SHOW SESSION {}'.format(session_id))
         self.check_resp_failed(resp, ttypes.ErrorCode.E_EXECUTION_ERROR)
-        resp = self.execute('UPDATE CONFIGS graph:session_idle_timeout_secs = 0')
+        resp = self.execute('UPDATE CONFIGS graph:session_idle_timeout_secs = 28800')
         self.check_resp_succeeded(resp)
         time.sleep(3)