github-actions[bot] commented on code in PR #67053:
URL: https://github.com/apache/doris/pull/67053#discussion_r3850064021
##########
fe/fe-core/src/main/java/org/apache/doris/qe/MysqlConnectProcessor.java:
##########
@@ -196,6 +196,7 @@ protected void handleExecute(PrepareCommand prepareCommand,
long stmtId, Prepare
// When audit log is disabled for prepared statements, still
update QPS metrics.
AuditLogHelper.updateMetrics(ctx);
}
+ executor.releaseAuditStatisticsOwnership();
Review Comment:
[P1] Suppress the proxy-side prepared audit before releasing this owner. For
a forwarded COM_STMT_EXECUTE with prepared auditing enabled, `auditAfterExec()`
above submits the master executor and completes its ownership transfer, so this
release and `handleProxy()`'s final release are both no-ops. The fragment
reports statistics to the connection/follower FE, which also audits the
returned result, leaving a duplicate master event that waits for fallback and
logs without counters. Keep the audit on the forwarding FE and add an
enabled-audit forwarded prepared-DML test that asserts exactly one event.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/AbstractInsertExecutor.java:
##########
@@ -265,6 +265,9 @@ private void checkStrictModeAndFilterRatio() throws
Exception {
*/
public void executeSingleInsert(StmtExecutor executor) throws Exception {
try {
+ // Nereids insert unregisters its coordinator before returning to
StmtExecutor, so the
+ // execution owner must exist before any terminal BE snapshot can
race that teardown.
+ executor.beginAuditStatisticsOwnership();
Review Comment:
[P1] Pair this owner with a terminal action for non-auditing callers.
`StreamingInsertTask`, `InsertTask`, `DictionaryManager`, and connector rewrite
invoke the command or `executeSingleInsert()` directly, so they never reach
`AuditLogHelper` or a `ConnectProcessor` release; the sibling unconditional
registration in `StmtExecutor.execute()` similarly affects recurring internal
DDL/DML helpers. Each call leaves an `inFlightAuditQueryReferences` entry
forever, and cleanup then permanently retains any accepted BE statistics for
that query ID. Scope ownership to an actual audit intent or guarantee final
release/transfer for every direct/internal entry, with repeated success/failure
tests that return both maps to zero.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/Coordinator.java:
##########
@@ -3842,6 +3843,10 @@ public List<TNetworkAddress> getInvolvedBackends() {
return backendAddresses;
}
+ public Set<Long> getInvolvedBackendIds() {
+ return ImmutableSet.copyOf(beToPipelineExecCtxs.keySet());
Review Comment:
[P2] Return only backends whose fragment RPC was actually submitted,
conservatively including uncertain submissions. `beToPipelineExecCtxs` is
filled for every planned worker before serialization and RPC; if serialization
fails or dispatch stops partway, this accessor includes BEs that can never
report. The failed DML audit then waits the 60-second fallback instead of the
normal short delay. Track a separate dispatch-participant set in both classic
and Nereids coordinators, and test zero- and partial-dispatch failures.
##########
fe/fe-core/src/main/java/org/apache/doris/planner/GroupCommitPlanner.java:
##########
@@ -152,6 +153,9 @@ public PGroupCommitInsertResponse
executeGroupCommitInsert(ConnectContext ctx,
throws DdlException, RpcException, ExecutionException,
InterruptedException, LoadException {
Backend backend =
Env.getCurrentEnv().getGroupCommitManager().selectBackendForGroupCommit(table.getId(),
ctx);
backendId = backend.getId();
+ // A cached full-prepare plan keeps its original load ID, which is the
identity used by BE
+ // runtime statistics; bind that identity before dispatch so audit
retention cannot race it.
+ bindAuditStatisticsIdentity(ctx, loadId, backendId);
Review Comment:
[P2] Publish this backend only once the group-commit BRPC has actually been
submitted, conservatively retaining uncertain submissions.
`BackendServiceProxy` still has to obtain or create the client after this call
and can throw synchronously before any request reaches the BE; the prepared
statement is then audited with this nonempty participant set and waits the
60-second fallback for a ResourceContext that never existed. Reserve the cached
load ID and ownership without a participant first, add the backend at the
dispatch boundary, and cover a synchronous client/channel failure.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/AuditLogHelper.java:
##########
@@ -408,7 +409,27 @@ private static void logAuditLogImpl(ConnectContext ctx,
String origStmt, Stateme
auditEventBuilder.setState(String.valueOf(MysqlStateType.OK));
}
AuditEvent event = auditEventBuilder.build();
-
Env.getCurrentEnv().getWorkloadRuntimeStatusMgr().submitFinishQueryToAudit(event);
+ Set<Long> expectedBackendIds = ImmutableSet.of();
+ String statisticsQueryId = event.queryId;
+ boolean transfersInFlightOwnership = false;
+ if (!event.isQuery && ctx.getExecutor() != null) {
+ // Audit completion is query-scoped: keep the event pending until
every backend that
+ // received a fragment publishes its final cumulative statistics
snapshot.
+ expectedBackendIds = ctx.getExecutor().getAuditBackendIds();
+ }
+ if (ctx.getExecutor() != null) {
+ String executorStatisticsQueryId =
ctx.getExecutor().getAuditStatisticsQueryId();
Review Comment:
[P1] Do not override a retried query audit with the first attempt's ID.
`execute(TUniqueId)` initializes this executor identity once, but
`handleQueryWithRetry()` can assign a fresh context query ID for attempt B
without resetting it. B's backends and audit event use the new ID while this
lookup still reads A, so the successful query loses or misattributes its
counters. Synchronize the identity on inner retries, or scope the alternate
identity to the explicit DML/group-commit case, and test failed attempt A
followed by successful B.
--
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]
