[feat](load) support cross-AZ minimum success replica quorum on load commit - #66680
[feat](load) support cross-AZ minimum success replica quorum on load commit#66680deardeng wants to merge 2 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
552efc5 to
80be1f9
Compare
|
run buildall |
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
FE UT Coverage ReportIncrement line coverage |
…commit
Add a cluster-level FE config `cross_az_succ_quorum` (format `az1:2,az2:1`,
empty by default) requiring, per tablet, a minimum number of successfully
written replicas in each availability zone before a load transaction may
commit. It only tightens the existing commit condition, never relaxes it; the
default empty value keeps today's behavior unchanged and costs nothing on the
commit path.
The requirement of each AZ is clamped to the replicas the tablet actually has
there:
required_in_az = min(configured, replica_num_in_az)
A tablet with no replica in a configured AZ (single-AZ tables, backends
without a location tag) is therefore skipped, and a table whose replica
distribution cannot reach the configured value degrades instead of becoming
permanently unloadable. This mirrors how `min_load_replica_num` is clamped in
OlapTable#getLoadRequiredReplicaNum.
FE enforces it in DatabaseTransactionMgr#checkCommitStatus, the single choke
point shared by the 2PC pre-commit, ordinary commit and sub-transaction commit
paths.
BE's quorum success write is made AZ aware as well. Without that, a merely
slow replica is dropped from the reported success set once the ordinary
majority is reached; in a cross-AZ deployment the systematically slowest
replica is the remote-AZ one, so the FE check would reject healthy loads. Two
optional thrift fields carry what BE needs -- `TNodeInfo.location` and
`TOlapTableSink.cross_az_succ_quorum` -- both sent only when the config is
non-empty, so an old BE simply ignores them and falls back to the FE-only
check. BE applies the identical clamp in
DorisNodesInfo#is_cross_az_quorum_success, shared by the v1 and v2 tablet
writers. Replicas with a version gap are excluded from the success count on
both sides.
Applies to the integrated storage-compute mode only; cloud mode commits
through CloudGlobalTransactionMgr and never reaches this check.
80be1f9 to
a739cc4
Compare
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: five blocking issues remain after the third and final review round. The accepted findings are: (1) both BE writers use destination completion instead of tablet-level success for the new AZ gate; (2) malformed configuration updates succeed and silently weaken or disable the durability fence; (3) the dynamic array/cache protocol does not safely publish updates to planner/commit threads; (4) remote Doris inserts have neither the target policy nor target backend locations in their planning metadata; and (5) new-FE/old-coordinator rolling deployments decode the optional fields but still reject healthy slow-AZ loads. Each is detailed inline.
Critical checkpoints:
- Goal, accomplishment, and proof: the goal is a per-tablet, per-AZ minimum-success durability fence with coordinated BE waiting and FE commit enforcement. The ordinary local path is implemented and the added tests exercise local commit rejection, clamping, a slow AZ,
min_load_replica_num, and part of v2 waiting. The five findings show the goal is not yet met across failure, dynamic-config, remote, and rolling-upgrade paths. - Scope and focus: the patch is reasonably focused, but the contract is distributed across configuration, planning, two BE writers, transaction commit, remote metadata, and thrift compatibility; the current small local changes do not cover those required boundaries. The user supplied no additional focus beyond full review.
- Concurrency: mutable-config updates race with planner and commit readers. The raw
String[]is reflectively written underConfigBase.class, read without that monitor, and the slow path locksConfig.class; this is MAIN-003. No new lock-order or deadlock issue was found. - Lifecycle and statics: no C++ static-initialization or ownership issue was found. The Java identity cache lifecycle is incorrect because its source reference is not safely published; that is covered by MAIN-003.
- Configuration: the setting is dynamic, but invalid values are accepted fail-open (MAIN-002), and successful updates need atomic publication (MAIN-003). Zero values and per-tablet clamping are intentional.
- Compatibility: the new thrift fields are wire-compatible in both directions, and old-FE/new-BE preserves the default behavior. New-FE/old-BE is not behaviorally compatible once a nonempty policy is activated, because the old coordinator cannot perform the slow-AZ wait (MAIN-007).
- Parallel and special paths: v1 and v2 are both changed but both share MAIN-001. Ordinary commit,
onlyCommit, subtransactions, 2PC precommit/final prepared-state handling, group commit's second master-planned writer, auto/replace-partition callbacks, cloud's one-replica topology, and target remote commit were traced. Remote planning remains broken (MAIN-005); no additional local commit bypass survived. - Conditions and errors: the AZ clamp matches FE/BE and missing tablet/node objects are rejected during writer initialization. Catching malformed policy items and continuing is not valid at a durability boundary (MAIN-002).
- Tests and results: coverage omits v1 and completed per-tablet failures, concurrent config publication, target remote policy/location provenance, and new-FE/old-BE activation. The invalid-config tests currently codify the fail-open result. Per the review bundle, no local build or test was run. Current CI shows compile, CheckStyle, and Clang Formatter passing; FE UT is failing, its linked log requires authorization, and BE/regression jobs are still pending.
- Observability: commit failures include transaction/tablet/AZ detail. Logging and ignoring an invalid update is insufficient because the administrative operation still reports success; no separate metrics blocker was found.
- Transactions, persistence, and failover: FE remains the authoritative commit fence and prevents an under-protected commit when it observes the policy. 2PC final commit correctly consumes the already-validated prepared state, and no new EditLog or master-failover defect was substantiated. MAIN-002/003 can nevertheless make the authoritative fence weaker or stale, and MAIN-005 splits policy provenance across clusters.
- Write atomicity and crashes: the existing commit protocol remains atomic; no new crash leak or visibility-version inconsistency was found. The accepted defects either weaken the intended durability rule or reject healthy loads after BE prematurely abandons a usable replica.
- FE/BE variable transfer: normal local sinks carry both optional fields, but remote backend metadata has no location and uses the source FE policy (MAIN-005); mixed-version coordinator capability is not fenced (MAIN-007).
- Performance: cached parsing and the per-tablet replica scans are bounded/linear and no material CPU, memory, or allocation regression was found.
- Other issues and completion: live review threads/comments were empty, all candidates were deduplicated, and the third/final round converged with every reviewer returning
NO_NEW_VALUABLE_FINDINGSafter two false positives were removed. The review is complete, not capped/incomplete.
### What problem does this PR solve? Issue Number: close #xxx Related PR: apache#66680 Problem Summary: Cross-AZ quorum checks treated a finished backend or stream destination as successful for every tablet, even when that destination reported a tablet-level failure. This could let a load return quorum success without the configured number of successful replicas in an availability zone. Count only finished destinations that actually succeeded for each tablet in both writer implementations, safely publish dynamic FE updates to cross_az_succ_quorum, and add regression coverage for both writers. ### Release note None ### Check List (For Author) - Test: Unit Test - `./run-be-ut.sh --run --filter=TestVTabletWriterV2.* -j 16` (19 tests passed) - `mvn checkstyle:check -pl fe-common` (passed) - `build-support/check-format.sh` with clang-format 16 (passed) - `build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN` (attempted; blocked by pre-existing diagnostics and a toolchain stddef.h lookup error) - Behavior changed: Yes, cross-AZ quorum now counts tablet-level write success and dynamic FE config updates are safely published across threads. - Does this need documentation: No
|
run buildall |
|
/review |
FE Regression Coverage ReportIncrement line coverage |
There was a problem hiding this comment.
Request changes: the authoritative FE gate prevents an under-replicated AZ result from being committed, but the end-to-end implementation can still reject or time out healthy loads in five supported lifecycle paths. The inline comments cover five distinct roots.
Critical checkpoint conclusions:
- Goal and data correctness: stable single-owner INSERT cases are covered, but the feature is not correct across in-flight policy/topology changes, multi-sender/source result ownership, staged auto-partition close, and transient replica repair. The failures occur before FE commit persistence, so I found no new invalid-data visibility or partition-version corruption path; the substantiated impact is healthy-load rejection, timeout, or premature replica cancellation.
- Scope and design: the diff is focused, but the contract spans planner metadata, optional Thrift fields, two BE writer protocols, and authoritative transaction validation. Correctness needs one immutable policy/topology/load-target epoch and a final tablet-result owner visible to every quorum evaluator.
- Concurrency and lifecycle: config and backend-tag updates are not covered by the transaction/table lock, which creates the snapshot mismatch. Writer result containers are lock-protected, but their final outcomes are sender/source-local. The initial/incremental close ordering and scheduler-added
CLONEstate create two further lifecycle failures. No additional lock-order, deadlock, use-after-free, or static-initialization issue was substantiated. - Configuration and compatibility: the volatile config source publishes each individual update, but it does not make planning and commit one snapshot. The appended optional Thrift fields decode compatibly. Existing live threads already cover malformed-config fail-open behavior, remote-insert metadata, and new-FE/old-BE activation; I did not duplicate them.
- Parallel paths, transactions, and persistence: ordinary, stream, routine, broker, and group-commit routing, plus ordinary commit, 2PC precommit/finalization, subtransactions, replay, failover, schema-change/rollup tablets, and the cloud-manager split were traced. Every fresh shared-nothing commit reaches the FE check before
PRECOMMITTED/COMMITTEDpersistence; no independent commit bypass or EditLog defect was found. - Error handling, performance, and observability: rejection errors contain transaction, tablet, table, partition, AZ, counts, and replica detail. Enabled-path scans are bounded in-memory work and backend tags are cached per check. I found no separate unchecked-status, heavy-I/O-under-lock, hot-path, or observability blocker.
- Tests: the added tests cover stable AZ failure/success, physical clamping, slow replicas, version gaps, parser caching, and lowered load quorum. They do not cover in-flight enable/disable or retagging, multi-sender v1 results, multi-source v2 results, mixed initial/incremental auto-partition close, or a loadable replica plus
CLONE; those gaps correspond directly to the five inline findings. - User focus and completion: no additional user-provided review focus was supplied. The review converged in round 2: all normal and risk-focused reviewers returned
NO_NEW_VALUABLE_FINDINGS, every candidate was adjudicated, and five distinct findings remain accepted. No builds or tests were run because this review environment disallows them.
| TabletInvertedIndex tabletInvertedIndex = env.getTabletInvertedIndex(); | ||
| Map<Long, Set<Long>> tabletToBackends = new HashMap<>(); | ||
| Map<Long, Table> idToTable = new HashMap<>(); | ||
| Map<String, Integer> crossAzSuccQuorum = Config.getCrossAzSuccQuorum(); |
There was a problem hiding this comment.
This rereads a mutable policy and live backend locations at commit, but BE close waiting used the policy and locations serialized when the sink was planned. A reachable failure is: plan with the default empty config, delay the only az2 replica, then enable az1:2,az2:1 before close/commit. BE has no AZ gate and may stop after the two az1 replicas; this fresh read then requires az2 and rejects an otherwise healthy load that would have succeeded if BE kept waiting. Disabling the config or changing a location tag in flight creates the inverse mismatch, and the separate reads inside setNodesInfo can even pair an enabled policy with location-less nodes. Please bind one immutable policy/location snapshot to the transaction/load and use it for node construction, sink metadata (including auto-partition updates), and FE validation; add blocked in-flight enable/disable coverage.
| const auto failed_it = _failed_channels.find(tablet_id); | ||
| for (int64_t node_id : tablet->node_ids) { | ||
| if (finished_node_ids.contains(node_id) && | ||
| (failed_it == _failed_channels.end() || |
There was a problem hiding this comment.
This still treats absence from this sender's _failed_channels as tablet success, but non-final senders never receive the final tablet outcome. TabletsChannel::close returns them OK with empty tablet_errors/tablet_vec; only the sender that closes the receiver last gets commit errors, and hang_wait does not copy that response back. Thus an earlier sender can count a destination whose tablet later fails in a required AZ, enter the bounded post-quorum wait, and cancel the only slow healthy replica before FE rejects. Please publish the final per-tablet result to every sender that evaluates quorum (or centralize the decision) and add a two-sender v1 case where the non-final sender closes first.
| if (finished) { | ||
| finished_dst_ids.insert(dst_id); | ||
| if (table_sink.__isset.cross_az_succ_quorum) { | ||
| for (int64_t tablet_id : streams->success_tablets()) { |
There was a problem hiding this comment.
success_tablets() is source-local, but the destination sends final tablet IDs only on whichever source's CLOSE_LOAD arrives last. Earlier sources receive an empty EOS and are only stream-closed later; the existing two-client load-stream test even expects only the second client to get the tablet ID. Consequently another source can have ordinary quorum and a genuinely satisfied AZ requirement, yet keep waiting for an unrelated slow destination because this map is empty, eventually timing out instead of using quorum close. Please share/broadcast final tablet outcomes to every source that evaluates this predicate (or evaluate it at the aggregated owner) and add a multi-source v2 close test.
| return false; | ||
| } | ||
| } | ||
| if (table_sink.__isset.cross_az_succ_quorum) { |
There was a problem hiding this comment.
This enforces the full AZ requirement during _close_wait(_non_incremental_streams(), false), before incremental streams are sent CLOSE_LOAD. For an auto-created tablet whose required-AZ success is available only on an incremental destination, that success cannot exist in this stage; if any extra initial stream is slow, the loop cannot use quorum and can run to the query timeout before reaching close_load(true). The v1 two-stage path has the same dependency. Please make the first-stage fence depend only on evidence that can finish in that stage and enforce the full AZ rule in the final all-stream wait; add auto-partition tests for both writers.
| for (Entry<String, Integer> entry : crossAzSuccQuorum.entrySet()) { | ||
| String az = entry.getKey(); | ||
| int replicaNumInAz = 0; | ||
| for (long backendId : tabletBackends) { |
There was a problem hiding this comment.
tabletBackends includes every replica object, including a scheduler-added CLONE, but load planning excludes CLONE because it cannot load. With one allocated az1 replica and az1:2, a same-AZ repair/balance temporarily leaves the loadable replica plus the new CLONE here, so requiredInAz becomes 2 although only one replica was a write target; every otherwise valid load is rejected until cloning finishes. Please clamp against the declared/logical replica allocation (or another count that excludes transient extra replicas) and add a one-replica-plus-CLONE test.
TPC-H: Total hot run time: 28649 ms |
TPC-DS: Total hot run time: 158278 ms |
ClickBench: Total hot run time: 25.13 s |
2 participants
Add a cluster-level FE config
cross_az_succ_quorum(formataz1:2,az2:1, empty by default) requiring, per tablet, a minimum number of successfully written replicas in each availability zone before a load transaction may commit. It only tightens the existing commit condition, never relaxes it; the default empty value keeps today's behavior unchanged and costs nothing on the commit path.The requirement of each AZ is clamped to the replicas the tablet actually has there:
A tablet with no replica in a configured AZ (single-AZ tables, backends without a location tag) is therefore skipped, and a table whose replica distribution cannot reach the configured value degrades instead of becoming permanently unloadable. This mirrors how
min_load_replica_numis clamped in OlapTable#getLoadRequiredReplicaNum.FE enforces it in DatabaseTransactionMgr#checkCommitStatus, the single choke point shared by the 2PC pre-commit, ordinary commit and sub-transaction commit paths.
BE's quorum success write is made AZ aware as well. Without that, a merely slow replica is dropped from the reported success set once the ordinary majority is reached; in a cross-AZ deployment the systematically slowest replica is the remote-AZ one, so the FE check would reject healthy loads. Two optional thrift fields carry what BE needs --
TNodeInfo.locationandTOlapTableSink.cross_az_succ_quorum-- both sent only when the config is non-empty, so an old BE simply ignores them and falls back to the FE-only check. BE applies the identical clamp inDorisNodesInfo#is_cross_az_quorum_success, shared by the v1 and v2 tablet writers. Replicas with a version gap are excluded from the success count on both sides.
Applies to the integrated storage-compute mode only; cloud mode commits through CloudGlobalTransactionMgr and never reaches this check.
Do not enable cross_az_succ_quorum until all potential coordinator BEs have been upgraded.
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)