github-actions[bot] commented on code in PR #63865:
URL: https://github.com/apache/doris/pull/63865#discussion_r3333467294
##########
fe/fe-core/src/main/java/org/apache/doris/load/DeleteHandler.java:
##########
@@ -144,6 +144,11 @@ public void process(Database targetDb, OlapTable
targetTbl, List<Partition> sele
deleteJob.await();
String commitMsg = deleteJob.commit();
execState.setOk(0, 0, commitMsg);
+ ConnectContext ctx = ConnectContext.get();
+ if (ctx != null) {
+ ctx.setOrUpdateInsertResult(deleteJob.getTransactionId(),
deleteJob.getLabel(),
+ targetDb.getFullName(), targetTbl.getName(),
TransactionStatus.VISIBLE, 0, 0);
Review Comment:
`DeleteJob.commit()` can return successfully with the transaction still in
`COMMITTED` when `commitAndPublishTransaction(...)` times out before publish
finishes; it even puts `status:'COMMITTED'` in `commitMsg` for that case. This
new insert result always records `TransactionStatus.VISIBLE`, so after a valid
committed-but-not-visible DELETE, `SHOW LAST INSERT` (and any later consumer of
`ctx.getInsertResult()`) reports VISIBLE while the transaction is not visible
yet. Please derive the status from the commit result/transaction state instead
of hardcoding VISIBLE.
##########
regression-test/suites/audit/test_audit_log_label_txn_id.groovy:
##########
@@ -0,0 +1,134 @@
+// 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.
+
+suite("test_audit_log_label_txn_id","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
+ }
+
+ sql "drop table if exists audit_label_txn_test"
+ sql """
+ CREATE TABLE `audit_label_txn_test` (
+ `id` bigint,
+ `name` varchar(32)
+ ) ENGINE=OLAP
+ UNIQUE KEY(`id`)
+ COMMENT 'OLAP'
+ DISTRIBUTED BY HASH(`id`) BUCKETS 1
+ PROPERTIES (
+ "replication_allocation" = "tag.location.default: 1"
+ )
+ """
+
+ def auditSchema = sql """desc internal.__internal_schema.audit_log"""
+ def columnNames = auditSchema.collect { it[0].toString().toLowerCase() }
+ assertTrue(columnNames.contains("label"), "audit_log should contain label
column")
+ assertTrue(columnNames.contains("txn_id"), "audit_log should contain
txn_id column")
+
+ sql "truncate table __internal_schema.audit_log"
+
+ def uniqueLabel = "audit_test_" +
UUID.randomUUID().toString().replace("-", "_")
+
+ // INSERT with label
+ sql "insert into audit_label_txn_test with label ${uniqueLabel} values (1,
'alice')"
+
+ // INSERT without explicit label (auto-generated)
+ sql "insert into audit_label_txn_test values (2, 'bob')"
+
+ // UPDATE
+ sql "update audit_label_txn_test set name = 'charlie' where id = 2"
+
+ // DELETE
+ sql "delete from audit_label_txn_test where id = 1"
+
+ // SELECT (should have empty label and -1 txn_id)
+ sql "select * from audit_label_txn_test"
+
+ Thread.sleep(6000)
+ sql """call flush_audit_log()"""
+
+ // Check INSERT with label has non-empty label and valid txn_id
+ def retry = 180
+ def insertWithLabelRes = sql "select label, txn_id from
__internal_schema.audit_log where stmt_type = 'INSERT' and label =
'${uniqueLabel}' order by time desc limit 1"
+ while (insertWithLabelRes.isEmpty()) {
+ if (retry-- < 0) {
+ throw new RuntimeException("It has retried a few but still failed
to find INSERT with label")
+ }
+ sleep(3000)
+ insertWithLabelRes = sql "select label, txn_id from
__internal_schema.audit_log where stmt_type = 'INSERT' and label =
'${uniqueLabel}' order by time desc limit 1"
+ }
+ assertTrue(insertWithLabelRes[0][0].toString().length() > 0, "INSERT with
label should have non-empty label")
+ assertTrue(Long.parseLong(insertWithLabelRes[0][1].toString()) > 0,
"INSERT should have valid txn_id")
+
+ // Check INSERT without explicit label still has auto-generated label and
valid txn_id
+ retry = 180
+ def insertNoLabelRes = sql "select label, txn_id from
__internal_schema.audit_log where stmt_type = 'INSERT' and stmt like
'%audit_label_txn_test%' and label != '${uniqueLabel}' order by time desc limit
1"
+ while (insertNoLabelRes.isEmpty()) {
+ if (retry-- < 0) {
+ throw new RuntimeException("It has retried a few but still failed
to find INSERT without label")
+ }
+ sleep(3000)
+ insertNoLabelRes = sql "select label, txn_id from
__internal_schema.audit_log where stmt_type = 'INSERT' and stmt like
'%audit_label_txn_test%' and label != '${uniqueLabel}' order by time desc limit
1"
+ }
+ assertTrue(insertNoLabelRes[0][0].toString().length() > 0, "INSERT without
explicit label should have auto-generated label")
+ assertTrue(Long.parseLong(insertNoLabelRes[0][1].toString()) > 0, "INSERT
should have valid txn_id")
+
+ // Check UPDATE has label and txn_id
+ retry = 180
Review Comment:
This UPDATE check is not scoped to the statement/table generated by this
test, and the DELETE check below has the same problem. `truncate
__internal_schema.audit_log` only clears persisted rows; it does not clear
events already buffered in the audit loader, so `call flush_audit_log()` can
reinsert older UPDATE/DELETE audit events from before the truncate. In that
case these queries can pass with an unrelated row and fail to verify that this
PR populated label/txn_id for `audit_label_txn_test`. Please filter
UPDATE/DELETE by this test's statement/table (or another unique marker) the
same way the INSERT-without-label check does.
--
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]
