From c078c6cdc5346ecadca3e53fbd00b18eadeee326 Mon Sep 17 00:00:00 2001 From: Yichen Wang <18348405+Aiee@users.noreply.github.com> Date: Thu, 8 Aug 2024 10:53:43 +0800 Subject: [PATCH] Fix session ID generation (#5916) * Fix session ID generation * Fix session create time * Fix tck cases * Fmt * Fix parser * Fix parser --- .../executor/admin/KillQueryExecutor.cpp | 2 +- .../session/SessionManagerProcessor.cpp | 11 +++++--- src/parser/parser.yy | 6 ++--- tests/tck/features/admin/Sessions.feature | 26 +++++++++---------- .../KillSlowQueryViaDiffrentService.feature | 4 +-- .../KillSlowQueryViaSameService.feature | 4 +-- .../PermissionViaDifferentService.feature | 12 ++++----- .../PermissionViaSameService.feature | 8 +++--- 8 files changed, 39 insertions(+), 34 deletions(-) diff --git a/src/graph/executor/admin/KillQueryExecutor.cpp b/src/graph/executor/admin/KillQueryExecutor.cpp index 55d827c6316..f59aeddf9b0 100644 --- 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 index 47449671ef2..52a3a53c218 100644 --- 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 + 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(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 index 27042a80db4..5a902606095 100644 --- 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 index 2b2ddc1dfa1..5944f3cd004 100644 --- 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 index bd049e943bf..1786359e176 100644 --- 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 index b911704d0e3..19c081fab68 100644 --- 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 index 066edc49152..75a1630cf58 100644 --- 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 index 5b4a66badb0..49061a1ff7a 100644 --- 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;