CalvinKirs commented on PR #70123: URL: https://github.com/apache/doris/pull/70123#issuecomment-6658976701
Reviewed `32063b0`. The core goal makes sense, and the write checks, effective-value reads, startup corrections, and receiving-FE ownership of forwarded prepared audits all have concrete reasons. I would address the following correctness issues before merging: 1. **An accepted audit timeout can stop every audit-table load.** The [load-timeout rule](https://github.com/apache/doris/blob/32063b01837b8bb49bd4452d99c998c41faed34e/fe/fe-common/src/main/java/org/apache/doris/qe/QueryAuditPolicy.java#L87-L89) only enforces a minimum of 600 seconds. With `query_audit_always_on=true` and the default stream-load limits, this is still accepted: ```sql SET GLOBAL audit_plugin_load_timeout = 259201; ``` The effective getter preserves that value and `AuditStreamLoader` sends it as the HTTP timeout. However, the [transaction validator](https://github.com/apache/doris/blob/32063b01837b8bb49bd4452d99c998c41faed34e/fe/fe-core/src/main/java/org/apache/doris/transaction/GlobalTransactionMgrIface.java#L49-L67) rejects it because `max_stream_load_timeout_second` defaults to 259200. Both the ordinary and cloud transaction managers apply this check. The [audit loader clears the batch](https://github.com/apache/doris/blob/32063b01837b8bb49bd4452d99c998c41faed34e/fe/fe-core/src/main/java/org/apache/doris/plugin/audit/AuditLoader.java#L285-L296) after the failed load, so subsequent batches keep being discarded until the setting is corrected. File audit still works. The new policy therefore accepts a protected setting that deterministically disables table audit. Please validate the effective timeout against the downstream supported range, including historical/replayed values, and add upper-b ound coverage. 2. **The new early-failure audit retains the previous statement's correlation fields.** The [fallback](https://github.com/apache/doris/blob/32063b01837b8bb49bd4452d99c998c41faed34e/fe/fe-core/src/main/java/org/apache/doris/qe/MysqlConnectProcessor.java#L320-L324) clears `QueryId` but leaves `SqlHash` and `StmtId`, which [AuditLogHelper copies into the event](https://github.com/apache/doris/blob/32063b01837b8bb49bd4452d99c998c41faed34e/fe/fe-core/src/main/java/org/apache/doris/qe/AuditLogHelper.java#L290-L293). On one connection, prepare A, execute an unrelated COM_QUERY B, then execute A with a binding or uploaded-long-data error. No new executor runs to assign A's identity, so the failed row contains A's SQL but B's hash and statement ID. Please clear/initialize those fields too, and capture the actual event in the test; mocking `logAuditLog` only checks that it was called. I also think the policy could be narrower in three places: - **Constrain effective behavior rather than each parameter's factory default.** The [INSERT rule](https://github.com/apache/doris/blob/32063b01837b8bb49bd4452d99c998c41faed34e/fe/fe-common/src/main/java/org/apache/doris/qe/QueryAuditPolicy.java#L83-L86) requires `Integer.MAX_VALUE`. With the default SQL cap of 2097152, setting the INSERT cap to 2097152 produces exactly the same effective cap through [min(insert, sql)](https://github.com/apache/doris/blob/32063b01837b8bb49bd4452d99c998c41faed34e/fe/fe-core/src/main/java/org/apache/doris/qe/AuditLogHelper.java#L151-L156), yet the policy rejects it. This restriction does not improve the recorded text. - **Keep batching requirements separate from keeping audit enabled.** The [fixed 50 MiB / 60-second settings](https://github.com/apache/doris/blob/32063b01837b8bb49bd4452d99c998c41faed34e/fe/fe-common/src/main/java/org/apache/doris/qe/QueryAuditPolicy.java#L90-L96) prevent ordinary workload tuning. For example, a low-QPS deployment flushing every 30 seconds does not reduce event selection or recorded text. The blanket assumption that any departure from the defaults weakens audit needs stronger justification. - **Avoid permanently rewriting saved globals solely for SHOW consistency.** [Master takeover journals the effective values](https://github.com/apache/doris/blob/32063b01837b8bb49bd4452d99c998c41faed34e/fe/fe-core/src/main/java/org/apache/doris/qe/VariableMgr.java#L564-L589), although the effective readers already enforce the policy. Turning the flag off does not restore the previous settings, and policy-off peers also replay these changes. The PR documents this, so it is an explicit design choice, but displaying configured/effective values would preserve the operator's settings and remove this migration path. Rewriting global defaults also does not make existing sessions' displayed values agree with effective behavior. I would split out the independently useful negative-SQL-length fix, retain the necessary always-on enforcement, and settle the numerical tuning policy and permanent-normalization requirement separately. The central policy abstraction is reasonable if the broad parameter scope remains; the main simplification opportunity is the scope of the rules. This was a source review; I did not run builds or tests. -- 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]
