Fix session ID generation (#5916)

* Fix session ID generation * Fix session create time * Fix tck cases * Fmt * Fix parser * Fix parser
vesoft-inc · Aug 8, 2024 · c078c6c · c078c6c
1 parent fee249b
commit c078c6c
Show file tree

Hide file tree

Showing 8 changed files with 39 additions and 34 deletions.
diff --git a/src/graph/executor/admin/KillQueryExecutor.cpp b/src/graph/executor/admin/KillQueryExecutor.cpp
@@ -79,7 +79,7 @@ Status KillQueryExecutor::verifyTheQueriesByLocalCache(QueriesMap& toBeVerifiedQ
 
     auto sessionId = sessionVal.getInt();
     auto epId = epVal.getInt();
-    if (sessionId == session->id() || sessionId < 0) {
+    if (sessionId == session->id() || sessionId == 0) {
       if (!session->findQuery(epId)) {
         return Status::Error("ExecutionPlanId[%ld] does not exist in current Session.", epId);
       }

diff --git a/src/meta/processors/session/SessionManagerProcessor.cpp b/src/meta/processors/session/SessionManagerProcessor.cpp
@@ -5,15 +5,20 @@
 
 #include "meta/processors/session/SessionManagerProcessor.h"
 
+#include <folly/Random.h>
+
 namespace nebula {
 namespace meta {
 
 void CreateSessionProcessor::process(const cpp2::CreateSessionReq &req) {
   auto user = req.get_user();
   cpp2::Session session;
-  // The sessionId is generated by microsecond timestamp
-  session.session_id_ref() = time::WallClock::fastNowInMicroSec();
-  session.create_time_ref() = session.get_session_id();
+  int64_t rand = 0;
+  while (rand == 0) {
+    rand = static_cast<int64_t>(folly::Random::rand64());
+  }
+  session.session_id_ref() = rand;  // 0 is used as a special value in kill query
+  session.create_time_ref() = time::WallClock::fastNowInMicroSec();
   session.update_time_ref() = session.get_create_time();
   session.user_name_ref() = user;
   session.graph_addr_ref() = req.get_graph_addr();

diff --git a/src/parser/parser.yy b/src/parser/parser.yy
@@ -3497,7 +3497,7 @@ show_sentence
     | KW_SHOW KW_LOCAL KW_SESSIONS {
         $$ = new ShowSessionsSentence(true);
     }
-    | KW_SHOW KW_SESSION legal_integer {
+    | KW_SHOW KW_SESSION unary_integer {
         $$ = new ShowSessionsSentence($3);
     }
     | KW_SHOW KW_META KW_LEADER {
@@ -3886,7 +3886,7 @@ kill_session_sentence
     ;
 
 query_unique_identifier_value
-    : legal_integer {
+    : unary_integer {
         $$ = ConstantExpression::make(qctx->objPool(), $1);
     }
     | input_prop_expression {
@@ -3896,7 +3896,7 @@ query_unique_identifier_value
 
 query_unique_identifier
     : KW_PLAN ASSIGN query_unique_identifier_value {
-        $$ = new QueryUniqueIdentifier($3, ConstantExpression::make(qctx->objPool(), Value(-1)));
+        $$ = new QueryUniqueIdentifier($3, ConstantExpression::make(qctx->objPool(), Value(0)));
     }
     | KW_SESSION ASSIGN query_unique_identifier_value COMMA KW_PLAN ASSIGN query_unique_identifier_value {
         $$ = new QueryUniqueIdentifier($7, $3);

diff --git a/tests/tck/features/admin/Sessions.feature b/tests/tck/features/admin/Sessions.feature
@@ -13,8 +13,8 @@ Feature: Test sessions
       SHOW SESSIONS;
       """
     Then the result should contain:
-      | SessionId | UserName | SpaceName | CreateTime | UpdateTime | GraphAddr | Timezone | ClientIp                            |
-      | /\d+/     | "root"   | ""        | /.*/       | /.*/       | /.*/      | 0        | /\b(?:[0-9]{1,3}\.){3}[0-9]{1,3}\b/ |
+      | SessionId  | UserName | SpaceName | CreateTime | UpdateTime | GraphAddr | Timezone | ClientIp                            |
+      | /[-+]?\d+/ | "root"   | ""        | /.*/       | /.*/       | /.*/      | 0        | /\b(?:[0-9]{1,3}\.){3}[0-9]{1,3}\b/ |
     When executing query:
       """
       CREATE USER user1 WITH PASSWORD 'nebula1';
@@ -29,9 +29,9 @@ Feature: Test sessions
       SHOW SESSIONS;
       """
     Then the result should contain, replace the holders with cluster info:
-      | SessionId | UserName | SpaceName | CreateTime | UpdateTime | GraphAddr                                           | Timezone | ClientIp                            |
-      | /\d+/     | "root"   | "s1"      | /.*/       | /.*/       | "127.0.0.1:${cluster.graphd_processes[0].tcp_port}" | 0        | /\b(?:[0-9]{1,3}\.){3}[0-9]{1,3}\b/ |
-      | /\d+/     | "user1"  | ""        | /.*/       | /.*/       | "127.0.0.1:${cluster.graphd_processes[1].tcp_port}" | 0        | /\b(?:[0-9]{1,3}\.){3}[0-9]{1,3}\b/ |
+      | SessionId  | UserName | SpaceName | CreateTime | UpdateTime | GraphAddr                                           | Timezone | ClientIp                            |
+      | /[-+]?\d+/ | "root"   | "s1"      | /.*/       | /.*/       | "127.0.0.1:${cluster.graphd_processes[0].tcp_port}" | 0        | /\b(?:[0-9]{1,3}\.){3}[0-9]{1,3}\b/ |
+      | /[-+]?\d+/ | "user1"  | ""        | /.*/       | /.*/       | "127.0.0.1:${cluster.graphd_processes[1].tcp_port}" | 0        | /\b(?:[0-9]{1,3}\.){3}[0-9]{1,3}\b/ |
 
   @distonly
   Scenario: Show local sessions
@@ -40,8 +40,8 @@ Feature: Test sessions
       SHOW SESSIONS;
       """
     Then the result should contain:
-      | SessionId | UserName | SpaceName | CreateTime | UpdateTime | GraphAddr | Timezone | ClientIp                            |
-      | /\d+/     | "root"   | ""        | /.*/       | /.*/       | /.*/      | 0        | /\b(?:[0-9]{1,3}\.){3}[0-9]{1,3}\b/ |
+      | SessionId  | UserName | SpaceName | CreateTime | UpdateTime | GraphAddr | Timezone | ClientIp                            |
+      | /[-+]?\d+/ | "root"   | ""        | /.*/       | /.*/       | /.*/      | 0        | /\b(?:[0-9]{1,3}\.){3}[0-9]{1,3}\b/ |
     When executing query:
       """
       CREATE USER IF NOT EXISTS user1 WITH PASSWORD 'nebula1';
@@ -61,14 +61,14 @@ Feature: Test sessions
       SHOW SESSIONS;
       """
     Then the result should contain, replace the holders with cluster info:
-      | SessionId | UserName | SpaceName    | CreateTime | UpdateTime | GraphAddr                                           | Timezone | ClientIp                            |
-      | /\d+/     | "root"   | "root_space" | /.*/       | /.*/       | "127.0.0.1:${cluster.graphd_processes[0].tcp_port}" | 0        | /\b(?:[0-9]{1,3}\.){3}[0-9]{1,3}\b/ |
-      | /\d+/     | "user1"  | ""           | /.*/       | /.*/       | "127.0.0.1:${cluster.graphd_processes[1].tcp_port}" | 0        | /\b(?:[0-9]{1,3}\.){3}[0-9]{1,3}\b/ |
-      | /\d+/     | "user2"  | ""           | /.*/       | /.*/       | "127.0.0.1:${cluster.graphd_processes[2].tcp_port}" | 0        | /\b(?:[0-9]{1,3}\.){3}[0-9]{1,3}\b/ |
+      | SessionId  | UserName | SpaceName    | CreateTime | UpdateTime | GraphAddr                                           | Timezone | ClientIp                            |
+      | /[-+]?\d+/ | "root"   | "root_space" | /.*/       | /.*/       | "127.0.0.1:${cluster.graphd_processes[0].tcp_port}" | 0        | /\b(?:[0-9]{1,3}\.){3}[0-9]{1,3}\b/ |
+      | /[-+]?\d+/ | "user1"  | ""           | /.*/       | /.*/       | "127.0.0.1:${cluster.graphd_processes[1].tcp_port}" | 0        | /\b(?:[0-9]{1,3}\.){3}[0-9]{1,3}\b/ |
+      | /[-+]?\d+/ | "user2"  | ""           | /.*/       | /.*/       | "127.0.0.1:${cluster.graphd_processes[2].tcp_port}" | 0        | /\b(?:[0-9]{1,3}\.){3}[0-9]{1,3}\b/ |
     When executing query:
       """
       SHOW LOCAL SESSIONS;
       """
     Then the result should contain, replace the holders with cluster info:
-      | SessionId | UserName | SpaceName    | CreateTime | UpdateTime | GraphAddr                                           | Timezone | ClientIp                            |
-      | /\d+/     | "root"   | "root_space" | /.*/       | /.*/       | "127.0.0.1:${cluster.graphd_processes[0].tcp_port}" | 0        | /\b(?:[0-9]{1,3}\.){3}[0-9]{1,3}\b/ |
+      | SessionId  | UserName | SpaceName    | CreateTime | UpdateTime | GraphAddr                                           | Timezone | ClientIp                            |
+      | /[-+]?\d+/ | "root"   | "root_space" | /.*/       | /.*/       | "127.0.0.1:${cluster.graphd_processes[0].tcp_port}" | 0        | /\b(?:[0-9]{1,3}\.){3}[0-9]{1,3}\b/ |
diff --git a/tests/tck/slowquery/KillSlowQueryViaDiffrentService.feature b/tests/tck/slowquery/KillSlowQueryViaDiffrentService.feature
@@ -30,8 +30,8 @@ Feature: Slow Query Test
       WHERE $-.DurationInUSec > 1000000 AND $-.`Query` CONTAINS "GO 100000 STEPS";
       """
     Then the result should be, in order:
-      | sid   | eid   | dur   |
-      | /\d+/ | /\d+/ | /\d+/ |
+      | sid        | eid   | dur   |
+      | /[-+]?\d+/ | /\d+/ | /\d+/ |
     # sessionId not exist
     When executing query via graph 1:
       """

diff --git a/tests/tck/slowquery/KillSlowQueryViaSameService.feature b/tests/tck/slowquery/KillSlowQueryViaSameService.feature
@@ -30,8 +30,8 @@ Feature: Slow Query Test
       WHERE $-.DurationInUSec > 1000000 AND $-.`Query` CONTAINS "GO 100000 STEPS";
       """
     Then the result should be, in order:
-      | sid   | eid   | dur   |
-      | /\d+/ | /\d+/ | /\d+/ |
+      | sid        | eid   | dur   |
+      | /[-+]?\d+/ | /\d+/ | /\d+/ |
     When executing query:
       """
       SHOW QUERIES

diff --git a/tests/tck/slowquery/PermissionViaDifferentService.feature b/tests/tck/slowquery/PermissionViaDifferentService.feature
@@ -32,8 +32,8 @@ Feature: Test kill queries permission from different services
       WHERE $-.DurationInUSec > 1000000 AND $-.`Query` CONTAINS "GO 100001 STEPS";
       """
     Then the result should be, in order:
-      | sid   | eid   | dur   |
-      | /\d+/ | /\d+/ | /\d+/ |
+      | sid        | eid   | dur   |
+      | /[-+]?\d+/ | /\d+/ | /\d+/ |
     # Kill failed by user test_permission
     When executing query with user "test_permission" and password "test":
       """
@@ -88,8 +88,8 @@ Feature: Test kill queries permission from different services
       WHERE $-.DurationInUSec > 1000000 AND $-.`Query` CONTAINS "GO 100002 STEPS";
       """
     Then the result should be, in order:
-      | sid   | eid   | dur   |
-      | /\d+/ | /\d+/ | /\d+/ |
+      | sid        | eid   | dur   |
+      | /[-+]?\d+/ | /\d+/ | /\d+/ |
     When executing query with user "test_permission" and password "test":
       """
       USE nba;
@@ -132,8 +132,8 @@ Feature: Test kill queries permission from different services
       WHERE $-.DurationInUSec > 1000000 AND $-.`Query` CONTAINS "GO 100003 STEPS";
       """
     Then the result should be, in order:
-      | sid   | eid   | dur   |
-      | /\d+/ | /\d+/ | /\d+/ |
+      | sid        | eid   | dur   |
+      | /[-+]?\d+/ | /\d+/ | /\d+/ |
     When executing query with user "root" and password "nebula":
       """
       USE nba;

diff --git a/tests/tck/slowquery/PermissionViaSameService.feature b/tests/tck/slowquery/PermissionViaSameService.feature
@@ -31,8 +31,8 @@ Feature: Test kill queries from same service
       WHERE $-.DurationInUSec > 1000000 AND $-.`Query` CONTAINS "GO 100001 STEPS";
       """
     Then the result should be, in order:
-      | sid   | eid   | dur   |
-      | /\d+/ | /\d+/ | /\d+/ |
+      | sid        | eid   | dur   |
+      | /[-+]?\d+/ | /\d+/ | /\d+/ |
     When executing query with user "test_permission" and password "test":
       """
       USE nba;
@@ -85,8 +85,8 @@ Feature: Test kill queries from same service
       WHERE $-.DurationInUSec > 1000000 AND $-.`Query` CONTAINS "GO 100002 STEPS";
       """
     Then the result should be, in order:
-      | sid   | eid   | dur   |
-      | /\d+/ | /\d+/ | /\d+/ |
+      | sid        | eid   | dur   |
+      | /[-+]?\d+/ | /\d+/ | /\d+/ |
     When executing query with user "test_permission" and password "test":
       """
       USE nba;