github-actions[bot] commented on code in PR #64599: URL: https://github.com/apache/doris/pull/64599#discussion_r3425740042
########## regression-test/suites/audit/test_audit_log_hint_session_context.groovy: ########## @@ -0,0 +1,71 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// A session variable changed by a per-query SET_VAR hint is temporary: it is reverted in +// StmtExecutor.execute()'s finally block. The audit log is built afterwards (in auditAfterExec), +// so without a pre-revert snapshot the hint value never reaches changed_variables. This case +// verifies that a per-query SET_VAR(session_context=...) is recorded in the audit log's +// changed_variables. The value is quoted so the hint parser keeps the ':' in the value. +suite("test_audit_log_hint_session_context", "nonConcurrent") { + try { + sql "set global enable_audit_plugin = true" + } catch (Exception e) { + log.warn("skip this case, because " + e.getMessage()) + assertTrue(e.getMessage().toUpperCase().contains("ADMIN")) + return + } + + def tbl = "audit_hint_session_context" + sql "drop table if exists ${tbl}" + sql """ + CREATE TABLE `${tbl}` (`id` bigint) ENGINE=OLAP + DUPLICATE KEY(`id`) + DISTRIBUTED BY HASH(`id`) BUCKETS 1 + PROPERTIES ("replication_allocation" = "tag.location.default: 1") + """ + sql "insert into ${tbl} values (1)" + + // unique markers so the exact statement can be located in the audit log + def hintTrace = "trace_id:hint_sc_7F3A2B" + def stmtMarker = "audit_hint_sc_marker_7F3A2B" + + sql "truncate table __internal_schema.audit_log" + + // per-query SET_VAR hint sets session_context for this statement only + sql """select /*+ SET_VAR(session_context="${hintTrace}") */ id, '${stmtMarker}' as marker from ${tbl}""" + + Thread.sleep(6000) + sql """call flush_audit_log()""" + + def retry = 180 + def query = """select ELEMENT_AT(changed_variables, 'session_context') + from __internal_schema.audit_log + where stmt like '%${stmtMarker}%' order by time desc limit 1""" + def res = sql "${query}" + while (res.isEmpty()) { Review Comment: This predicate can match the audit row for the polling query itself, because the SQL text of this `select ELEMENT_AT(...)` query also contains `${stmtMarker}` and the pattern is unanchored. If the target row is delayed past the first poll, a later poll can return the newer audit row for the lookup statement with no `session_context`, causing a null/mismatch instead of testing the SET_VAR query. Please narrow the predicate to the statement under test, for example by also requiring `stmt like 'select /*+ SET_VAR(session_context=%'`. ########## regression-test/suites/audit/test_audit_log_hint_session_context.groovy: ########## @@ -0,0 +1,71 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// A session variable changed by a per-query SET_VAR hint is temporary: it is reverted in +// StmtExecutor.execute()'s finally block. The audit log is built afterwards (in auditAfterExec), +// so without a pre-revert snapshot the hint value never reaches changed_variables. This case +// verifies that a per-query SET_VAR(session_context=...) is recorded in the audit log's +// changed_variables. The value is quoted so the hint parser keeps the ':' in the value. +suite("test_audit_log_hint_session_context", "nonConcurrent") { + try { + sql "set global enable_audit_plugin = true" + } catch (Exception e) { + log.warn("skip this case, because " + e.getMessage()) Review Comment: Please use `setGlobalVarTemporary([enable_audit_plugin: true]) { ... }` (as the neighboring audit test does) or restore the original value in a `finally`. As written, any failure after this line leaves a global setting changed for later suites, and the final `set global enable_audit_plugin = false` also overwrites an originally enabled state. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
