[feature](fe) Add cross-AZ success quorum check - #66751
Open
deardeng wants to merge 2 commits into
Open
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#66680 Problem Summary: Load transaction commits only enforced the ordinary replica quorum and could therefore succeed without a configured minimum number of successful replicas in each availability zone. Add a mutable FE configuration and enforce the per-AZ success floor in the centralized transaction commit check. Clamp each AZ requirement to the partition declared replica allocation so backend liveness changes and transient extra replicas do not weaken or inflate the commit requirement. ### Release note Add the mutable FE configuration cross_az_succ_quorum to require a minimum number of successful load replicas per availability zone. ### Check List (For Author) - Test: Unit Test - DatabaseTransactionMgrTest cross-AZ quorum, unavailable replica, and extra replica cases - Behavior changed: Yes (when cross_az_succ_quorum is configured, FE rejects commits that do not meet the per-AZ success floor) - 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes because the opt-in cross-AZ durability policy is not end-to-end safe yet.
Blocking findings:
- The FE adds a distribution-specific check after both BE writer generations have already finalized commit infos using only the ordinary scalar quorum, so a healthy slower required-AZ replica can be omitted permanently.
- Startup and mutable config updates accept malformed or duplicate policy entries, while the lazy parser silently drops or overwrites them and enforces a weaker policy.
- Commit classification reads a backend location tag that
MODIFY BACKENDmutates without safe publication, so it can enforce the policy against stale topology.
Critical checkpoint conclusions:
- Goal and proof: The local FE check expresses the requested floor, but the three findings mean the end-to-end goal is not accomplished; direct FE tests with fabricated commit infos do not prove the writer-to-commit path.
- Scope and clarity: The three-file patch is locally focused and readable, but the required planner/BE producer handoff and validation boundary are missing.
- Concurrency and locks: Config cache publication is safe for the supported replace-on-update path. Table locks stabilize partition/index metadata, but not backend tag publication; no additional lock-order or deadlock issue was found.
- Lifecycle and statics: The process-static cache has a coherent replacement lifecycle, with no circular initialization or resource-release issue.
- Dynamic configuration: The master observes replacement arrays promptly, but malformed values fail open. Master-only failover behavior otherwise matches adjacent mutable-config semantics.
- Compatibility: No persisted or symbol format changes are introduced, but current and older BEs receive no AZ-aware completion criterion or capability fence, so rolling deployment is unsafe when the feature is enabled.
- Parallel paths: Ordinary non-cloud transaction sources, local 2PC at precommit, and each subtransaction reach
checkCommitStatus; final 2PC correctly preserves the prepared decision. Cloud uses a distinct meta-service transaction model. Auto-partition is exposed to the same writer handoff defect. - Conditions: Allocation clamping and the zero-requirement skip match the declared-allocation policy; dead, bad, missing, decommissioned, extra, and loaded schema-change replicas yielded no separate defect.
- Test coverage: The added tests cover direct success/failure, clamping, unavailable/bad replicas, and an extra replica. They miss end-to-end v1/v2 delayed-remote behavior, startup/admin rejection, and the backend-topology concurrency boundary.
- Test results: Compile, CheckStyle, and several static checks are green; FE UT and regression/performance jobs were still pending at review time. No local tests were run because this workflow is review-only.
- Observability: Failure logs include transaction, tablet, table, partition, version, AZ, and replica detail. No separate logging/metrics blocker was found.
- Transactions and persistence: The check runs before commit or precommit state advancement; no edit-log payload is added, and prepared-state failover semantics remain intact.
- Writes and crashes: Existing atomic state transitions remain in place and no new leak/crash path was found, but the missing producer contract can turn a healthy load into a terminal commit failure.
- FE/BE variables: The AZ requirement is not passed or translated into an equivalent requirement on either initial or auto-partition writer paths; this is the first blocking finding.
- Performance: Parsing is amortized once per config replacement and backend tags are cached once per commit; no distinct material performance problem was found.
- Other issues and user focus: No additional user focus was supplied, and the full-PR sweep found no valuable issue beyond the three inline findings.
| if (!crossAzSuccQuorum.isEmpty()) { | ||
| backendLocationTags.computeIfAbsent(tabletBackend, backendId -> { | ||
| Backend backend = env.getCurrentSystemInfo().getBackend(backendId); | ||
| return backend == null ? "" : backend.getLocationTag().value; |
Contributor
There was a problem hiding this comment.
Consume a safely published backend-location snapshot here. MODIFY BACKEND calls setTagMap on an already-published Backend, but locationTag is non-volatile and unsynchronized, and that update does not republish the volatile backend map; this commit thread shares neither the writer's monitor nor another happens-before edge. After an az1 backend is relabeled to az2, a stale az1 read can count one success in each zone although both current successes are in az2. Publish the location through an immutable/volatile snapshot or shared synchronization, and test concurrent relabel/commit behavior around the defined snapshot boundary.
Contributor
Author
There was a problem hiding this comment.
This is intentional feature behavior. Invalid entries are skipped with a warning, while valid entries remain effective. We do not want one malformed entry to reject the entire dynamic configuration update.
### What problem does this PR solve? Issue Number: None Related PR: apache#66751 Problem Summary: Backend location tags can be updated by MODIFY BACKEND while transaction commit threads read them. The plain locationTag reference had no Java memory-model publication edge, so readers could observe a stale availability-zone tag. Publish replacement Tag instances through a volatile reference and add a deterministic regression test for the required publication semantics. ### Release note None ### Check List (For Author) - Test: Unit Test - BackendTest#testLocationTagIsSafelyPublished - Behavior changed: No (ensures existing backend location updates are visible across threads) - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
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
2 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: #66680
Problem Summary: Load transaction commits only enforced the ordinary replica quorum and could therefore succeed without a configured minimum number of successful replicas in each availability zone. Add a mutable FE configuration and enforce the per-AZ success floor in the centralized transaction commit check. Clamp each AZ requirement to the partition declared replica allocation so backend liveness changes and transient extra replicas do not weaken or inflate the commit requirement.
Release note
Add the mutable FE configuration cross_az_succ_quorum to require a minimum number of successful load replicas per availability zone.
Check List (For Author)
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)