Repository navigation
Conversation
### What problem does this PR solve?
Problem Summary: At verbose log levels, FragmentMgr formats complete pipeline parameters and external query plans. File scan properties, table descriptors and transaction parameters can contain storage credentials, passwords or tokens. Replace these dumps with a shared summary of query and fragment identifiers and instance counts, and keep only identifiers in the external open log. Execution parameters remain unchanged.
### Release note
Verbose fragment execution logs no longer include full execution parameters, query options or external query plans that may contain credentials.
### Check List (For Author)
- Test: clang-format 16 and whitespace checks passed; focused BE unit tests and ASAN build are in progress
- Unit Test: add synthetic credential coverage and capture both pipeline and external request logs
- Behavior changed: Yes; verbose diagnostics contain identifiers and counts instead of full parameter dumps
- Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
This was referenced Oct 23, 2026
OIiveirra
marked this pull request as ready for review
October 23, 2026 14:03
Collaborator
Author
|
run buildall |
Collaborator
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed #69540 at c17cf35 using the supplied aggregate diff and all five changed files. No actionable new findings were identified. There are no existing inline findings to retain as blockers. No additional user focus was supplied.
Checkpoint conclusions:
- Goal and correctness: the four FragmentMgr execution-parameter, query-options and external-plan dump statements are removed. The shared const formatter reads only numeric query/fragment IDs and the local-instance count, preserving execution inputs. Five unit tests cover exact output, default fields, nonmutation and synthetic credential omission, including the two production request paths.
- Scope and reuse: one formatter is shared by both execution summaries; changes remain limited to verbose logging and focused tests.
- Concurrency and locks: production adds no shared state or locks. Runtime test log capture protects its buffer with a mutex; teardown stops and joins FragmentMgr before removing the sink and restoring logging/debug-point configuration.
- Lifecycle and initialization: no new production static initializer, retained reference or resource owner. The fixture restores the state it changes; private ExecEnv construction follows the existing BE test configuration.
- Configuration: no configuration item added; the existing VLOG(10) and VLOG(1) levels are retained.
- Compatibility: no existing interface, execution/storage format, or OS/libc API changes. The generated type is forward-declared in the header and included in the implementation.
- Thrift/protobuf: no schema, field ID, enum value or requiredness changes; rolling-upgrade wire behavior is unchanged.
- Parallel paths: ordinary and callback RPC dispatch, transactional/direct callers and external scanner requests reach the updated core logging path. External open retains query/instance IDs and upstream original/new-query correlation.
- Conditional checks and errors: no production branch or Status handling changes; the tests intentionally stop at the existing pre-context failure hook after request logging.
- Test coverage: CMake automatically includes both new test files. Tests inspect known and unknown scan-property values, descriptor credentials, transaction tokens, counts, defaults and parameter equality. Runtime capture reaches the changed pipeline and external log sites.
- Expected results: exact summary assertions agree with numeric ID formatting and local-instance counts; no regression output files are changed. Tests were inspected, not run during this review.
- Observability: query/fragment/count summaries and external instance correlation remain available. Removing full parameters and options intentionally reduces verbose detail; no new execution path requires metrics.
- Transactions and persistence: transaction state, version visibility, delete bitmaps, metadata persistence and failover logic are unchanged.
- Writes and atomicity: no data-write, commit or cleanup behavior changes.
- FE/BE variables: no new transmitted variable or sending-point update.
- Performance and memory: bounded scalar formatting replaces recursive request serialization and remains lazily evaluated inside VLOG; no significant retained allocation or new memory-accounting path.
- Other review obligations: no new cache/factory ownership, nullable-column handling, cross-TU initialization, template instantiation, source-file inclusion or build-gate exception. No further introduced issue found after the missed-issue recheck.
Limits: this was a static review; no builds, tests, sanitizers, end-to-end scans or final ELF audits were executed, and the PR author's runtime evidence was not independently reproduced. The runtime assertions would not detect reintroducing only the separate query-options dump because they do not place a marker in query_options; the current source does remove that dump. An unchanged HTTP stream-load parameter dump remains outside this FragmentMgr-specific change. No unresolved candidate or uncovered changed-file scope remains.
Review converged after 2 round(s).
Contributor
TPC-H: Total hot run time: 28429 ms |
Contributor
TPC-DS: Total hot run time: 150400 ms |
Contributor
ClickBench: Total hot run time: 24.27 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
yiguolei
pushed a commit
that referenced
this pull request
Oct 24, 2026
#69543) ### What problem does this PR solve? Related PR: #69540 Problem Summary: FragmentMgr prints complete pipeline execution parameters at VLOG(10), including file-scan properties, descriptor passwords and transaction tokens. The external open path also prints a complete query plan at VLOG(1). Replace these dumps with query/fragment identifiers and instance counts without modifying execution parameters. ```diff - Log complete execution parameters and external query plans + Log fixed identifiers and instance counts ``` ### Release note Verbose fragment execution logs no longer include full execution parameters, query options or external query plans that may contain credentials. ### Evidence - `FragmentParamsLogTest.*:FragmentMgrLogTest.*`: **5 executed, 5 passed, 0 failed, 0 skipped** (Clang21/ASAN). Fresh GTest XML and unchanged source inputs were checked. - Tests cover synthetic S3/token/password values, unknown property keys, unchanged execution parameters, and real pipeline/external request log capture with VLOG(10) enabled. - clang-format 16 and whitespace checks passed. - Task-private build selection retained the two requested test files, native initialization/link support, and complete branch BE libraries. This validates the selected tests; full native UT and regression groups were not run. ### Check List (For Author) - Test: - [x] Unit Test: the five focused fragment log tests above. - Behavior changed: - [x] Yes. Verbose diagnostics contain fixed identifiers and counts instead of full parameter dumps. - Does this need documentation? - [x] No. ### Merge Danger **Door:** two-way **Blast Radius:** diagnostics Execution and protocol parameters are unchanged; only verbose log contents change. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
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?
Problem Summary: FragmentMgr prints complete pipeline execution parameters at VLOG(10), including file-scan properties, descriptor passwords and transaction tokens. The external open path also prints a complete query plan at VLOG(1). Replace these dumps with query/fragment identifiers and instance counts without modifying execution parameters.
Release note
Verbose fragment execution logs no longer include full execution parameters, query options or external query plans that may contain credentials.
Evidence
FragmentParamsLogTest.*:FragmentMgrLogTest.*: 5 executed, 5 passed, 0 failed, 0 skipped (Clang21/ASAN with the documented private legacy compatibility overlay). Fresh GTest XML and unchanged source inputs were checked.Check List (For Author)
Merge Danger
Door: two-way
Blast Radius: diagnostics
Execution and protocol parameters are unchanged; only verbose log contents change.
Check List (For Reviewer who merge this PR)