[refactor](nereids) Remove minidump, plan replayer, and tracing - #67811
Open
morrySnow wants to merge 2 commits into
Open
[refactor](nereids) Remove minidump, plan replayer, and tracing#67811morrySnow wants to merge 2 commits into
morrySnow wants to merge 2 commits into
Conversation
morrySnow
requested review from
924060929,
englefly and
starocean999
as code owners
September 10, 2026 11:40
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morrySnow
changed the title
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
924060929
previously approved these changes
Sep 12, 2026
924060929
left a comment
924060929
left a comment
Contributor
There was a problem hiding this comment.
Reviewed at a5bb3aa. The removal is cohesive: parser/command and REST entry points, session/config state, replay-only authorization and statistics branches, and tracing hooks are removed consistently. I found no remaining references or behavioral changes outside the retired interfaces.
### What problem does this PR solve? Issue Number: None Related PR: apache#66817 Problem Summary: The remaining minidump dump path and Nereids trace framework are no longer needed after offline plan replay was removed. Remove PLAN REPLAYER DUMP and its REST endpoint, minidump serialization, replay-only statistics and context plumbing, the enable_nereids_trace variables, and the event tracing implementation. Also remove replay-specific privilege and row-policy bypasses so normal planning always uses catalog statistics and standard authorization paths. ### Release note Remove `PLAN REPLAYER DUMP`, the `/api/minidump` endpoint, and the `enable_nereids_trace` and `nereids_trace_event_mode` session variables. ### Check List (For Author) - Test: Unit Test - `NereidsParserTest` - `UserAuthenticationTest` - Behavior changed: Yes. Remove the minidump, plan replayer dump, and Nereids trace interfaces. - Does this need documentation: No. These developer-only interfaces are not documented.
morrySnow
force-pushed
the
codex/remove-minidump-plan-replayer-nereids-trace
branch
from
September 14, 2026 03:55
a5bb3aa to
756d323
Compare
Contributor
Author
|
run buildall |
Contributor
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.
Request changes: one compatibility regression remains; see the inline comment.
Static review at exact head 756d3234912486b7df6e73d37a16a66ee6998133:
- Goal, scope, and tests: the command, REST action, minidump/tracer state, replay-only branches, configs, and tests are removed cohesively; parser coverage checks command rejection and identifier reuse, and the changes stay focused on that removal. The author reports
NereidsParserTestandUserAuthenticationTest(94 tests); I did not run builds or tests because the review contract prohibits it. No regression result files change, and compatibility coverage for the retired variables is missing. - Control flow, lifecycle, concurrency, and performance: ordinary query/explain/HTTP-stream/error, rewrite/memo/cost/enforcer, and statistics paths retain their former non-replay behavior. Removing the global tracer queue, counters, retained plans, JSON, and file I/O reduces shared-state and hot-path overhead.
- Compatibility, configuration, persistence, protocols, and data correctness: old FE config keys and persisted unknown fields are ignored. No FE-BE protocol, edit-log/storage format, transaction, data-write, or visibility invariant changes. The exception is the two removed public session variables, whose missing retired-name registrations break live
SET/SET_VAR/SELECT @@compatibility (inline). - Security: the replay-only policy and privilege bypasses disappear while normal RBAC and SQL-cache revalidation remain, strengthening the modeled boundary rather than introducing a new authorization issue.
- Observability: minidump/tracing removal is intentional; ordinary plan-process, profile, debug-log, and memo-dump diagnostics remain.
- User focus: none was supplied. At submission, CheckStyle, BE UT, Cloud UT, and coverage pass; COMPILE and FE UT are still pending.
Contributor
TPC-H: Total hot run time: 16852 ms |
Contributor
TPC-DS: Total hot run time: 82747 ms |
Contributor
ClickBench: Total hot run time: 14.88 s |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#67811 Problem Summary: Removing the Nereids trace @VarAttr registrations made upgraded FEs treat legacy SET and SELECT @@ traffic as unknown system variables. Preserve both retired names in the removed-session-variable compatibility set and cover primary and forwarded SET plus old-planner and Nereids reads. ### Release note Preserve no-op compatibility for the removed enable_nereids_trace and nereids_trace_event_mode session variables. ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.qe.VariableMgrTest - Behavior changed: Yes, legacy accesses to the removed trace variables remain compatible no-ops. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: one binary-artifact closure issue remains; see the inline comment.
Static review at exact head ef2cd1599fcd137491b90e22a7de662051e5f9aa:
- Goal, scope, and tests: the source-level removal of the command, REST action, minidump/tracing state, replay-only branches, configs, and fixtures is cohesive. Parser tests cover command rejection and
REPLAYERidentifier reuse; compatibility tests cover the two retired session-variable names. The author reports the focused parser, authorization, and variable tests; I did not run builds or tests because the review contract prohibits it. - Control flow and data correctness: ordinary analyze/rewrite/memo/statistics/cost/enforcer/translation behavior remains the former non-replay path. The flattened statistics logic is equivalent for normal queries, and no FE-BE protocol, edit-log/storage format, transaction, data-write, or visibility invariant changes.
- Lifecycle, concurrency, and performance: removing the global tracer state, daemon queue, retained plans, event counters, JSON generation, and dump I/O removes shared mutable state and hot-path diagnostic work. No new lock, allocation, or failure-path concern remains.
- Compatibility and configuration: old FE config keys and serialized unknown session fields are ignored safely. The exact-head fix preserves case-insensitive direct/forwarded
SET,SET_VAR, legacy read, and Nereids read compatibility for both retired trace variables; that earlier discussion is duplicate-fenced. The remaining issue is binary build-output compatibility on the standard non-clean FE packaging path (inline). - Security: removal of replay-only policy and privilege bypasses leaves ordinary RBAC behavior unchanged and strengthens surviving planner entry points. No new modeled authorization boundary issue was found.
- Observability: loss of minidump/tracing is intentional; normal plan-process traces, profiles, debug logs, and memo dumps remain.
- User focus: none was supplied. At submission, CheckStyle, BE UT, Cloud UT, and coverage pass; FE UT is red in two untouched HDFS/Iceberg tests, while compile, performance, and review checks remain pending.
| // Usage: | ||
| // wget http://fe_host:fe_http_port/api/minidump?query_id=123456 | ||
| @RestController | ||
| public class MinidumpAction extends RestBaseController { |
Contributor
There was a problem hiding this comment.
[P2] Clean deleted FE classes before incremental packaging
./build.sh --fe defaults to a non-clean Maven package, so on a cache-miss build after checking out this head over a base-built tree, javac leaves this deleted controller and MinidumpUtils under fe/fe-core/target/classes, and the jar plugin packages them again. Spring then still registers /api/minidump; invoking it initializes the stale utility against the new Config and throws NoSuchFieldError for spilled_minidump_storage_path. Please make the standard FE packaging path remove the deleted class outputs or otherwise force a clean for this transition, and verify the resulting jar no longer contains them.
Contributor
TPC-H: Total hot run time: 17013 ms |
Contributor
TPC-DS: Total hot run time: 82506 ms |
Contributor
ClickBench: Total hot run time: 14.73 s |
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
3 participants
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: #66817
Problem Summary:
The remaining minidump dump path and Nereids trace framework are no longer needed after offline plan replay was removed. Remove
PLAN REPLAYER DUMPand its REST endpoint, minidump serialization, replay-only statistics and context plumbing, theenable_nereids_tracevariables, and the event tracing implementation. Also remove replay-specific privilege and row-policy bypasses so normal planning always uses catalog statistics and standard authorization paths.Release note
Remove
PLAN REPLAYER DUMP, the/api/minidumpendpoint, and theenable_nereids_traceandnereids_trace_event_modesession variables.Check List (For Author)
Test
./run-fe-ut.sh --run org.apache.doris.nereids.parser.NereidsParserTest,org.apache.doris.nereids.rules.analysis.UserAuthenticationTest(94 tests)Behavior changed:
Does this need documentation?