[improvement](fe) Improve audit logs for S3 streaming insert jobs - #67489
Open
JNSimba wants to merge 2 commits into
Open
[improvement](fe) Improve audit logs for S3 streaming insert jobs#67489JNSimba wants to merge 2 commits into
JNSimba wants to merge 2 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
/review |
Member
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Adds internal query audit logging for S3 streaming insert tasks so audit events capture resolved S3 file paths (post-wildcard expansion) while masking sensitive TVF properties; also records audit events for both success and failure cases. CDC streaming insert behavior remains unchanged (no internal audit events).
Changes:
- Emit internal INSERT audit events for S3 streaming insert tasks with rewritten S3 URI and masked TVF properties.
- Ensure failed S3 streaming inserts also generate error audit events.
- Add regression/unit tests validating S3 audit emission and CDC non-emission.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| fe/fe-core/src/main/java/org/apache/doris/job/extensions/insert/streaming/StreamingInsertTask.java | Adds audit-enabled path for S3 streaming inserts, constructs masked/re-written audit SQL, and logs audit events on completion/failure. |
| fe/fe-core/src/test/java/org/apache/doris/job/extensions/insert/streaming/StreamingInsertTaskAuditTest.java | Introduces tests to assert S3 audit events are emitted (success/failure) and CDC tasks do not emit audit events. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review status: complete after round 2 convergence. I am requesting changes for two distinct audit-correctness defects described inline.
Critical checkpoints:
- Goal and functional correctness: S3-only gating, success/error state capture, and CDC exclusion are generally coherent, but valid S3 inputs can either lose the required audit event or retain the unresolved wildcard beside the resolved URI.
- Lifecycle, concurrency, and error handling: I traced setup, each retry attempt, success, existing and synthesized ERR states, cancellation, cleanup, and asynchronous event ownership. Each command attempt reaches the audit finally block, the public audit helper contains its own failures, and the queued event does not retain task-owned context. No additional distinct lifecycle defect remained.
- SQL reconstruction and sensitive data: recognized credential aliases remain masked, but the rewritten property map is a display representation rather than reparsable SQL. This causes the P1 event-loss finding; case-preserving URI rewriting causes the separate P2 accuracy finding.
- Scope, parallel paths, compatibility, and performance: CDC behavior is unchanged. No persistence, protocol, configuration, migration, or material performance regression is introduced by this two-file change.
- Tests: the new unit tests cover basic S3 OK/ERR events and CDC non-emission, but they inject a lowercase, syntax-neutral rewritten map and therefore miss both accepted defects. Per the review-run instructions, I did not run builds locally. Live CheckStyle passed; FE UT, compile, and performance checks were still pending at submission time.
- Existing review context: all three existing inline threads were treated as duplicate fences and were not repeated.
User focus: no additional focus was supplied, so the full PR was reviewed.
Contributor
TPC-H: Total hot run time: 16893 ms |
Contributor
TPC-DS: Total hot run time: 82110 ms |
Contributor
ClickBench: Total hot run time: 14.51 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Overall: request changes. One distinct P1 remains on the reviewed head; previously raised quote escaping, cancellation/finally isolation, replacement cardinality, URI case handling, and CDC-test concerns were treated as duplicate fences and were not repeated.
Finding
- The new audit reconstruction mixes ANTLR code-point coordinates with Java UTF-16 string indexes. A valid supplementary-Unicode S3 key can therefore suppress the audit event for a successful load or leave its final statement malformed.
Checkpoint conclusions
- Goal/data correctness and special cases: ordinary BMP-only S3 success/error attempts preserve final state, timestamp, executor statistics, resolved file URI, and sensitive-value masking. The Unicode range failure prevents the audit feature from being correct for all admitted S3 object keys.
- Concurrency, lifecycle, and transactions: each retry builds fresh context/command/executor/audit state, and auditing happens after the INSERT execution path. The concrete cancellation cleanup race is already covered by an existing thread. No additional transaction, lock-order, or unbounded-retention issue survived review.
- Configuration, compatibility, storage, and performance: no configuration, persisted format, FE/BE protocol, rolling-upgrade, or storage-layout change is introduced. The additional parsing/rendering is per S3 batch; no separate performance defect was substantiated.
- Tests: the new FE unit test covers S3 success/failure events, internal classification, resolved URI, masking, timestamp, URI case folding, and unchanged CDC behavior. Its reflective setup and ASCII-only fixture miss the reported Unicode failure. Per the review-runner instructions, I did not run builds or tests.
- Scope, parallel paths, and focus: the production change is focused on S3 audit emission; CDC remains unchanged. No additional user-provided review focus was supplied, so the full PR was reviewed.
Review completeness: complete after two rounds of whole-PR and risk-focused review; no unresolved candidates remain.
Contributor
TPC-H: Total hot run time: 16576 ms |
Contributor
TPC-DS: Total hot run time: 82031 ms |
Contributor
ClickBench: Total hot run time: 14.65 s |
Member
Author
|
run p0 |
1 similar comment
Member
Author
|
run p0 |
Contributor
FE Regression Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
S3 streaming insert tasks execute internal INSERT statements without emitting query audit events. This makes it difficult to identify the exact files imported after wildcard paths are resolved.
This change emits internal audit events only for S3 streaming insert tasks. The audited statement contains the rewritten S3 URI and masks sensitive TVF properties. Both successful and failed executions are recorded. CDC streaming insert tasks keep the existing behavior and do not emit these audit events.
Release note
S3 streaming insert jobs now record internal INSERT audit logs with resolved file paths and masked credentials.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)