[fix](replica) Resolve conflicting default replica properties#65836
Merged
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: ALTER TABLE updates to default.replication_allocation could leave the legacy default.replication_num property in table metadata. Because replica analysis checks replication_num first, SHOW CREATE TABLE and later consumers could observe the wrong default allocation. Remove the mutually exclusive property when applying an update, prefer replication_allocation when cleaning historical metadata, and cover allocation updates, metadata cleanup, and reverse numeric updates.
### Release note
Fix conflicting default replica properties so the latest explicit replica setting is used.
### Check List (For Author)
- Test: Unit Test
- ./run-fe-ut.sh --run org.apache.doris.catalog.TablePropertyTest (10 passed)
- cd fe && mvn checkstyle:check -pl fe-core (0 violations)
- git diff --cached --check
- Behavior changed: Yes (conflicting default replica properties are removed so the latest explicit setting wins)
- Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 25th, 2026 3:35 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 29378 ms |
Contributor
TPC-DS: Total hot run time: 177657 ms |
Contributor
ClickBench: Total hot run time: 24.77 s |
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.
Automated review result: request changes (2 blocking correctness/compatibility issues).
Critical checkpoint conclusions:
- Goal and tests: The prospective SQL ALTER path now canonicalizes either default-replica spelling and same-version journal replay is symmetric, but the full latest-explicit-setting goal is not met. Historical image cleanup can discard a newer numeric value, and restore can bypass the new mutual-exclusion path. The added unit tests cover direct forward/reverse map updates and one allocation-newer deserialization case, but not a numeric-newer upgrade image or the restore setter.
- Scope and abstraction: The patch is locally small, but the invariant is not centralized across all replica-property mutation APIs; that incompleteness causes the restore finding.
- Concurrency and lifecycle: No new race or lock-order problem was found; live and replay mutation use the table write lock, while restore prepares staging metadata. The nontrivial image-deserialization and restore/replay lifecycle boundaries are where both accepted issues occur.
- Persistence, compatibility, and parallel paths: SQL ALTER and its edit-log replay are prospectively equivalent. No storage-format or FE-BE protocol change is introduced. Historical snapshot precedence and mixed-version restore reconstruction are not compatible, as detailed inline.
- Conditions and error handling: Single-key allocation and numeric updates remove the intended opposite key. The both-key allocation-wins condition is not safe for undated historical state. No additional exception/status-handling defect was found.
- Configuration, data writes, FE-BE propagation, observability, and performance: No configuration, user-data transaction, or transmitted FE-BE variable is changed. The added map checks are constant time, and no new metric/log is warranted.
- Test execution: Per the review-only runner contract, no local build or test was run. Current GitHub status shows COMPILE and Checkstyle passing, but FE UT, P0 Regression, and check_coverage_fe failing; the linked TeamCity logs require authentication, so those failures remain unresolved.
- User focus: No additional user-provided focus was specified; the whole PR and the persistence/compatibility boundaries were reviewed.
Please address both inline findings and add the upgrade/restore coverage before merge.
### What problem does this PR solve? Issue Number: None Related PR: apache#65836 Problem Summary: Historical table metadata can contain both default replica properties without recording which value was written last. Unconditionally preferring replication allocation during image loading can therefore replace a newer numeric replica value. Restore also writes an allocation directly and can leave a legacy numeric property behind. Preserve the historical numeric-first image behavior, remove the numeric property from direct allocation setters, and make internal table replica updates persist the canonical allocation form. ### Release note Preserve historical default replica behavior during upgrades and prevent restore from persisting conflicting replica properties. ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.catalog.TablePropertyTest (11 passed) - ./run-fe-ut.sh --run org.apache.doris.alter.InternalSchemaAlterTest (2 passed) - cd fe && mvn checkstyle:check -pl fe-core (0 violations) - git diff --cached --check - Behavior changed: Yes. Historical images retain numeric-first precedence, while restore and future internal table updates persist a single canonical allocation property. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review result: request changes (1 blocking correctness/compatibility issue).
Critical checkpoint conclusions:
- Goal and tests: The current head fixes the two previously reported image-precedence and restore-setter issues and makes latest-explicit-setting behavior correct on a homogeneous current FE cluster. It does not preserve that behavior across mixed-version journal replay. Added tests cover both replacement directions, ambiguous image precedence, restore canonicalization, and partitioned/unpartitioned internal-table scaling, but not mixed-version replay/failover.
- Scope and abstraction: The source change is small and focused, and current-version mutation is centralized. The durable transition remains incomplete because removed keys are absent from its payload.
- Concurrency and lifecycle: No new race, lock-order, or heavy-under-lock issue was found; live and replay mutations use table write locks. The failure is at the rolling-upgrade replay/failover lifecycle boundary.
- Persistence, compatibility, and parallel paths: Same-version leader/replay behavior is equivalent. Mixed versions are not: old and current FEs can derive different defaults from the same journal, and the same primitive is used by parallel partition/colocate transitions. This is the blocking inline issue.
- Conditions, error handling, configuration, data writes, FE-BE propagation, observability, and performance: Caller canonicalization makes simultaneous-key current ALTER input unreachable; no additional status/exception defect was found. No configuration, user-data transaction, FE-BE field, storage format, metric, or material performance change is involved.
- Test execution: Per the review-only runner contract, no local build or test was run. GitHub compilation and checkstyle passed at review time, but FE UT reported failure; its TeamCity log requires authentication, so the failure could not be classified here.
- User focus: No additional user-provided focus was specified; the entire PR and its persistence/compatibility boundaries were reviewed.
Please make the removal replay-safe across supported rolling upgrades and add mixed-version replay/failover coverage before merge.
Contributor
TPC-H: Total hot run time: 29527 ms |
Contributor
TPC-DS: Total hot run time: 177462 ms |
Contributor
ClickBench: Total hot run time: 24.99 s |
gavinchou
approved these changes
Jul 22, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
Contributor
FE UT 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.
Problem Summary: ALTER TABLE updates to default.replication_allocation could leave the legacy default.replication_num property in table metadata. Because replica analysis checks replication_num first, SHOW CREATE TABLE and later consumers could observe the wrong default allocation. Remove the mutually exclusive property when applying an update, prefer replication_allocation when cleaning historical metadata, and cover allocation updates, metadata cleanup, and reverse numeric updates.
Release note
Fix conflicting default replica properties so the latest explicit replica setting is used.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)