github-actions[bot] commented on code in PR #66281:
URL: https://github.com/apache/doris/pull/66281#discussion_r3682155303
##########
fe/fe-core/src/main/java/org/apache/doris/qe/AuditLogHelper.java:
##########
@@ -413,6 +413,13 @@ private static long getQueueTimeMs(ConnectContext ctx) {
return queueToken == null ? -1 : queueToken.getQueueEndTime() -
queueToken.getQueueStartTime();
}
+ static String getCloudClusterForAudit(ConnectContext ctx) throws
ComputeGroupException {
+ if (!Strings.isNullOrEmpty(ctx.getEffectiveCloudCluster())) {
Review Comment:
[P1] Resolve empty overrides before treating the snapshot as absent
`cloud_cluster` is an unconstrained String variable, so
`SET_VAR(cloud_cluster='')` is valid. With session group A and cached/default
group B, the empty override makes `CloudCoordinator` fall back to B for
execution. The finally block records `""` and reverts the session to A, then
this condition treats the real snapshot as absent and falls back to A. The
audit row and per-cluster query/error/latency metrics are therefore charged to
A although B ran the query. Track snapshot presence separately and retain the
resolved execution group.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -672,6 +672,7 @@ boolean shouldDisableCloudVersionCacheOnRetry(String
errorMessage) {
public void execute(TUniqueId queryId) throws Exception {
SessionVariable sessionVariable = context.getSessionVariable();
+ context.setEffectiveCloudCluster(null);
Review Comment:
[P1] Clear this snapshot before pre-executor audit paths
This reset happens only after a `StmtExecutor` exists. After a hinted
statement leaves the snapshot at B and reverts the session to A, a following
malformed `COM_QUERY` fails in `parseWithFallback` and reaches
`handleQueryException -> auditAfterExec` before entering this method; neither
`processOnce`, `ctx.clear()`, nor connection reset clears the field. That error
is therefore audited under B. Reset at every top-level audit-capable statement
boundary (before parsing/early returns), or make the value statement-owned
instead of connection-owned.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/AuditLogHelper.java:
##########
@@ -448,7 +455,7 @@ private static void updateMetricsImpl(ConnectContext ctx) {
String physicalClusterName = "";
try {
if (Config.isCloudMode()) {
- cloudCluster = ctx.getCloudCluster(false);
+ cloudCluster = getCloudClusterForAudit(ctx);
Review Comment:
[P2] Retain the physical group selected for a virtual group
`CloudCoordinator.prepare` resolves a virtual group to a physical backend
set before execution, but this changed audit/metric path preserves only the
logical name and calls the health-dependent `getPhysicalCluster` again
afterward. If active/standby changes during a long query—or execution and audit
occur on FEs with different current views—the second lookup can return B even
though the query ran on A, charging B's query/error/latency metrics. Capture
the physical execution group at scheduling/forwarding time and use that
immutable value for physical metrics.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -698,6 +699,11 @@ public void execute(TUniqueId queryId) throws Exception {
throw e;
}
} finally {
+ // Preserve the effective per-query compute group before SET_VAR
values are reverted.
+ // Audit logging runs after this method returns and otherwise sees
the session value.
+ if (Config.isCloudMode()) {
+
context.setEffectiveCloudCluster(sessionVariable.getCloudCluster());
Review Comment:
[P1] Preserve the inner prepared execution's compute group
For `COM_STMT_EXECUTE`, the outer executor runs an `ExecuteCommand`, and
`ExecuteCommand.run` installs the saved plan on the same executor and
recursively calls `execute()`. The inner finally captures the hinted group and
then reverts SET_VAR, but control returns to this outer finally, which executes
this assignment again with the restored session group. Prepared audit (when
enabled) and `AuditLogHelper.updateMetrics` (when disabled) therefore still use
the wrong group. Make this capture statement/depth-owned so the wrapper cannot
overwrite the inner execution's snapshot.
##########
regression-test/suites/audit/test_audit_log_hint_session_context.groovy:
##########
@@ -75,5 +75,40 @@ suite("test_audit_log_hint_session_context",
"nonConcurrent") {
// The per-query SET_VAR hint session_context must be visible in
changed_variables.
assertTrue(found >= 1)
+ if (isCloudMode()) {
Review Comment:
[P1] Run this assertion in a cloud-executed suite
Because this suite carries `nonConcurrent`, the regression framework
classifies it as `SINGLE`. The cloud_p0 runner passes `-runNonConcurrent false`
and returns before SINGLE suites, while the standalone nonConcurrent deployment
is shared-nothing, so this `isCloudMode()` branch is never exercised in
standard CI. The new unit test only seeds `effectiveCloudCluster` directly and
cannot catch capture/revert or metric-path bugs. Please move this to an
isolated cloud multi-cluster test (or provide equivalent integration coverage)
that actually executes the hinted audit and metric lifecycle.
--
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]
