[opt](planner) Enable Nereids distribute planner after upgrade - #66604
Open
924060929 wants to merge 1 commit into
Open
[opt](planner) Enable Nereids distribute planner after upgrade#66604924060929 wants to merge 1 commit into
924060929 wants to merge 1 commit into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: New installations default enable_nereids_distribute_planner to true, but clusters upgrading from a variable version below 400 restore the persisted global session default from their image. Clusters that persisted the previous false default would therefore continue using the legacy distribute planner after upgrading to 4.0. Add enable_nereids_distribute_planner to the existing variable-version-400 migration so that the 3.x-to-4.0 upgrade enables it. The migration uses the existing global-variable edit-log path and does not change clusters whose variable version is already 400.
### Release note
Clusters upgrading to 4.0 now enable the Nereids distribute planner by default.
### Check List (For Author)
- Test: Unit Test
- `VariableMgrTest#testEnableNereidsDistributePlannerAfterUpgrade`
- FE Checkstyle for fe-core
- Behavior changed: Yes. Clusters upgrading from a variable version below 400 default to the Nereids distribute planner.
- Does this need documentation: No
924060929
force-pushed
the
agent/enable-nereids-distribute-planner-on-upgrade
branch
from
August 10, 2026 07:38
05770e2 to
2e5e4cc
Compare
924060929
marked this pull request as ready for review
August 10, 2026 07:38
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
morrySnow
changed the title
morningman
approved these changes
Aug 10, 2026
Contributor
|
PR approved by at least one committer and no changes requested. |
github-actions
Bot
added
approved
Contributor
|
PR approved by anyone and no changes requested. |
![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. The patch is focused, but it does not update the released upgrade state it is meant to fix, and the new test leaves durable global-variable side effects that a later checkpoint can replay.
Critical checkpoint conclusions:
- Goal and proof: The intended result is to enable the Nereids distribute planner for upgraded clusters. The implementation only handles a direct version-300-to-this-head path; released 4.0/4.1 clusters already persist version 400, so the real already-upgraded state is unchanged. The test starts at 300 and therefore does not prove the affected path.
- Scope and clarity: The production change is small and reuses the established migration helper, but it is attached to a spent version fence. The test is otherwise focused, but invoking the real durable migration without isolating its journal writes creates order-dependent global state.
- Concurrency: Migration runs during master promotion after journal replay, before serving readiness, and each update uses the existing
VariableMgrwrite lock. No new lock-order, deadlock, or concurrent-session mutation issue was found. - Lifecycle: This is a one-shot persisted migration. Target-before-version ordering is idempotent across crashes, but the already-consumed 400 lifecycle gate prevents the target update from running for existing 4.x images/journals.
- Configuration: No new configuration item is introduced; the existing global session default remains dynamically settable after the one-time migration.
- Compatibility and parallel paths: The variable and edit-log opcode already exist, and the sole production caller plus follower replay path were inspected. A fresh 401 fence is rolling-upgrade safe because older 4.x FEs can replay the known variable and the integer version, then skip their older
< 400migrations. No alternate production path repairs the current omission. - Special condition: The new
< VARIABLE_VERSION_400placement is the blocking condition. It needs a new version fence rather than<= 400or an unconditional rewrite, so later administrator changes remain preserved. - Test coverage and results: The new assertions are correct for the version-300 scenario but miss version 400 to the new fence and a post-migration no-op. The test also journals unrelated static-global changes that its in-memory
finallycannot survive a later checkpoint replay. No build or test was run in this review-only runner, per its instructions. - Observability: Existing upgrade logs identify the source version, variable, and value; no additional metric is needed for this one-time path.
- Persistence and failover:
OP_GLOBAL_VARIABLE_V2write/replay is symmetric, and logging the target before the version gives safe retry behavior. The defect is reachability, not serialization or failover ordering. - Data writes and FE/BE propagation: No transaction/data-write, storage-format, or new FE-to-BE variable surface is introduced.
- Performance and other risks: The work is constant and upgrade-only, with no hot-path cost. No additional distinct correctness, compatibility, lifecycle, or performance issue survived two review rounds.
User focus: no additional focus was provided; the full two-file PR was reviewed.
| VariableMgr.refreshDefaultSessionVariables(updateInfo, | ||
| SessionVariable.ENABLE_SQL_CACHE, | ||
| String.valueOf(true)); | ||
| VariableMgr.refreshDefaultSessionVariables(updateInfo, |
Contributor
There was a problem hiding this comment.
This extends a migration fence that released clusters have already consumed. A 3.1 image can contain planner=false/version=300; released 4.0 migrates that image to version 400 without changing the flag. On a later upgrade, replay restores false/400 before forceUpdateVariables(), so 400 < 400 is false and this assignment never runs. Please add a fresh one-shot version (for example, VARIABLE_VERSION_401, made current, with this assignment under < 401) and make the regression start from version 400 plus planner=false. Otherwise already-upgraded clusters remain on the legacy planner.
| defaultSessionVariable.setEnableNereidsDistributePlanner(false); | ||
| GlobalVariable.variableVersion = GlobalVariable.VARIABLE_VERSION_300; | ||
|
|
||
| VariableMgr.forceUpdateVariables(); |
Contributor
There was a problem hiding this comment.
This test journals the real version-400 migration, including unrelated ANSI/type-coercion changes, but finally restores them only in memory. If it precedes testGlobalVariablePersist, that test's edit_log_roll_num=1 finalizes these records and its manual checkpoint replays them. Checkpoint SessionVariable objects are isolated, but GlobalVariable contexts still target shared statics (obj=null), so ANSI/type-coercion become false again after this cleanup and leak into later methods. Please use a scoped edit-log spy/stub restored in finally, or otherwise make cleanup durable.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29083 ms |
Contributor
TPC-DS: Total hot run time: 158738 ms |
Contributor
ClickBench: Total hot run time: 23.97 s |
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
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: None
Problem Summary: New installations default enable_nereids_distribute_planner to true, but clusters upgrading from a variable version below 400 restore the persisted global session default from their image. Clusters that persisted the previous false default would therefore continue using the legacy distribute planner after upgrading to 4.0. Add enable_nereids_distribute_planner to the existing variable-version-400 migration so that the 3.x-to-4.0 upgrade enables it. The migration uses the existing global-variable edit-log path and does not change clusters whose variable version is already 400.
Release note
Clusters upgrading to 4.0 now enable the Nereids distribute planner by default.
Check List (For Author)
VariableMgrTest#testEnableNereidsDistributePlannerAfterUpgrade