Repository navigation
[fix](fe) Exclude load-disabled backends from load plans - #69554
HonestManXin wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes for two verified findings: a P1 regression in metadata callers of the shared replica enumeration, and a P2 cloud load-routing regression during graceful backend shutdown. The metadata finding combines incomplete light-schema conversion and colocate discovery/replay because they share the same changed API and remedy.
Reviewed all three changed files and their upstream/downstream load, migration, cloud, metadata and persistence paths at head 734ddac. No existing inline threads or previously reported blocking findings were present. No additional user focus was supplied.
| Critical checkpoint | Conclusion |
|---|---|
| Goal and proof | The predicate excludes alive load-disabled backends and retains an enabled DECOMMISSION replica before its post watermark. The added unit test covers those two cases; indirect callers cause the findings below. |
| Scope and clarity | The helper and test follow adjacent patterns and the patch is small, but the shared enumeration also serves metadata operations (P1). |
| Concurrency and locking | ALTER/backend-heartbeat writers publish volatile load/shutdown flags and atomic alive state; backend maps and local replica membership use volatile immutable snapshots. No new lock, blocking operation, lock-order edge or demonstrated race is introduced. Existing table-lock ownership of migration state is retained. |
| Lifecycle and initialization | No new owned resource, thread, retained state or static initializer. The existing live-but-shutting-down lifecycle exposes the P2 cloud routing defect. |
| Configuration | No new configuration. Existing disable_load is read dynamically for fresh maps; existing transaction retry caches intentionally preserve recorded locations. |
| Compatibility | No new wire/storage format or removed symbol. Old-master colocate journal replay can diverge on this head through the changed physical-replica enumeration (P1). |
| Thrift/protobuf IDs | No gensrc changes; cross-branch field-ID review is inapplicable. |
| Parallel paths | Normal sink plans, base/changelog intersection, cloud cluster/endpoint variants and create/replace-partition RPCs share the filter. Metadata callers need different eligibility (P1); cloud colocated/multi-replica selection needs rerouting (P2). |
| Conditional checks | Backend.isLoadAvailable centralizes alive/load-disabled/shutdown rules. Backend decommission itself is not excluded. Existing post-watermark and failed-DECOMMISSION exclusions remain intact. |
| Test coverage | The JUnit 5 test uses fresh backend/replica state and checks {1,3}. It lacks non-load metadata, cloud shutdown, post-watermark, failed/disabled DECOMMISSION, reenable and missing/dead backend cases; no end-to-end test was added. |
| Test results | No result files changed; the new expectation matches the fixture and distinguishes the prior alive-only behavior. Tests were not executed. |
| Observability | Existing load-quorum errors identify affected tablets/backends. No new long-lived operation requires metrics; false conversion success and skipped replay are part of P1. |
| Transactions and persistence | Migration peer-catch-up and both watermark waits remain; publication does not use this new load gate. Metadata conversion journals incomplete IDs and colocate replay can skip metadata (P1). |
| Data writes and invariants | Sink and commit still enforce required replicas; excluded replicas follow existing error/version handling. No direct visible-version, MoW bitmap or memory-accounting change. Persistent metadata correctness is affected by P1; availability by P2. |
| FE/BE variable passing | No new transmitted variable. Existing tablet-location backend ID sets are filtered across fresh load-map construction paths. |
| Performance | The existing per-replica lookup/scan is retained with constant-time flag reads; no added IO, RPC, lock or collection scan. No performance measurement was performed. |
| Other issues | Cloud disable_load via MODIFY BACKEND is unsupported, so that hypothetical trigger was dismissed. Ordinary non-colocated cloud routing already rehashes shutdown nodes. No additional verified issue remains after the caller and special-case recheck. |
Validation and coverage limits: static source review only, as required; no builds, runtime tests, source modifications or executed migration/shutdown interleavings. The two inline findings are supported by concrete control-flow triggers. No unresolved candidate or unreviewed changed file remains.
Review converged after 2 round(s).
TPC-H: Total hot run time: 28338 ms |
TPC-DS: Total hot run time: 150802 ms |
ClickBench: Total hot run time: 24.36 s |
FE Regression Coverage ReportIncrement line coverage |
734ddac to
0c4461a
Compare
|
/review |
There was a problem hiding this comment.
Request changes for one new P1: automatic partition RPCs can return an empty filtered destination list, allowing the supported legacy writer to report success while omitting an entire partition's rows. The inline finding covers the common createPartition/replacePartition failure boundary. No duplicate comments are proposed.
Existing findings were independently revalidated against 0c4461a: P1 #4361269076 is addressed because metadata/replay callers still use alive-only enumeration; P2 #4361269082 is addressed because all CloudTablet load overloads preserve the previous physical-backend selectors. No existing P0/P1 remains applicable. No additional user focus was supplied.
The traced flow is: load planning -> replica eligibility -> tablet locations -> ordinary or incremental BE writer channels -> EOS commit infos -> FE partition/quorum validation. All seven changed files and their relevant upstream/downstream and parallel paths were reviewed.
Critical checkpoint conclusions:
- Goal and proof: ordinary shared-nothing load plans now exclude load-disabled/shutdown backends, while metadata and cloud behavior are preserved. Dynamic empty destinations violate data correctness and error-means-failure (the P1). New unit tests cover positive routing; they do not prove the dynamic negative case.
- Scope and reuse: focused API split, existing Backend.isLoadAvailable predicate, and shared changelog intersection; no unrelated source change identified.
- Concurrency and locks: planning/RPC threads read heartbeat/DDL-updated atomic/volatile backend state and volatile immutable replica/backend snapshots. New filtering mutates a fresh map. Existing table snapshots, overwrite task locks and transaction/partition computeIfAbsent caches retain their scope; no new lock order, blocking IO or heavyweight work under locks.
- Lifecycle: no new thread, callback owner, persistent reference or static initializer; map ownership remains request-local.
- Configuration: none added. Existing backend status changes are visible to new uncached selection; cached in-flight routing intentionally remains stable.
- Compatibility: additive internal Java APIs; no removed symbol, stored format or wire change. Metadata replay and cloud endpoint/cluster routing are preserved.
- Thrift/protobuf: no definition changed; cross-branch field-ID checks are not applicable.
- Parallel paths: ordinary, changelog, automatic create, replacement, cloud overloads and group-commit routing were traced. Legacy and V2 writers differ on empty locations; V2 rejection does not protect the legacy path.
- Conditional checks: decommission post-watermark/failed-version rules remain intact; backend decommission itself is not rejected by isLoadAvailable. Same-disk changelog intersection and documented cloud bypass remain sound. Dynamic warn-only quorum handling is unsafe for empty mappings.
- Test coverage: added JUnit tests cover metadata/load separation, a decommission replica and cloud forwarding. Empty/sub-quorum dynamic RPCs, actual shutdown and filtered changelog pairs lack new negative coverage. The sink test fixture also leaves partition.getId() unstubbed, bypassing its intended quorum mock, although routing assertions remain meaningful.
- Test results: no golden outputs changed; new assertions agree with statically inspected paths. No builds, tests or runtime reproduction were performed, as required by this review contract.
- Observability: ordinary diagnostics now correctly name available replicas and retain tablet/count details. Dynamic warnings do not propagate failure, as described in the P1; no separate metrics change is needed for the local predicate split.
- Transactions and persistence: no journal schema or replay mutation introduced. FE commit checks all tablets only within partitions represented by commit infos; this allows the P1's wholly omitted partition. Existing failover/cache lifecycle is unchanged.
- Writes and atomicity: ordinary planning and V2 reject unusable targets; nonempty sub-quorum partitions fail FE commit. Legacy empty-target partition omission can commit partial ingestion (P1). No separate crash, leak, deadlock, visible-version or delete-bitmap regression was substantiated.
- FE/BE variables: none added; existing tablet/node/base-tablet fields carry the filtered mappings on all changed sending paths.
- Performance: O(replica-count) filtering over request-local maps; no added RPC, large allocation or heavier metadata critical section. Cloud cluster-ID hoisting is preserved.
- Further issues and limits: rechecked map ownership, dispatch, watermarks, placement eligibility, EOS writer enumeration, row counters, cache cohort consistency and commit partition derivation. One verified new finding; remaining test limitations are stated above. Validation is static only; no unresolved in-scope candidate remains after the final reviewer pass.
Review converged after 2 round(s).
0c4461a to
8d9cf2e
Compare
|
/review |
There was a problem hiding this comment.
Request changes on head 8d9cf2e. One new P1 is reported inline for initial load plans with reduced write quorum. The existing dynamic-partition P1 remains blocking; its overlapping RPC instance is not reposted.
Existing-thread dispositions:
- 4361269076 is fixed: metadata callers still enumerate alive normal replicas; only the explicit load API applies load eligibility.
- 4361269082 is fixed: all CloudTablet load overloads delegate to the previous resolved routes and preserve graceful-drain behavior.
- 4366121929 is retained for residual whole-partition omission on createPartition/replacePartition. Its original immediate-empty RF1 trigger is fixed. With default RF3/quorum2, however, a filtered nonempty location A->{1} still passes the new guard while another partition B->{1,4,5} is usable. If BE1's tablet-writer-open RPC fails, the legacy writer counts only one failure against its unchanged allowance of one; excluded BEs2/3 never count as failures. B can commit through4/5 while A produces no commit infos and is omitted from FE's partition checks. Both RPCs still only log subquorum locations. This expands the existing thread's evidence rather than adding a duplicate comment.
Reviewed flow: initial insert/stream-load planning or dynamic create/replace RPC -> filtered TTabletLocation node lists -> legacy channels / V2 streams -> successful tablet commit infos -> FE infers involved partitions -> quorum checks within those partitions. The failure is that a wholly failed filtered partition can disappear before the final check. The new initial-planner instance uses supported min_load_replica_num=1 and requires no dynamic partition RPC. The analogous V2 tablet-level CLOSE failure also uses the configured-total failure budget, so empty-stream rejection alone does not establish safety.
Critical checkpoint conclusions:
- Goal and proof: load-disabled shared-nothing targets are excluded while migration watermarks are preserved, but end-to-end correctness is incomplete because filtered targets are absent from failure accounting. Added tests cover selection and immediate-empty rejection, not the verified mixed-partition failure.
- Scope and reuse: the explicit load API and shared empty-map helper are small and reuse existing predicates; no unrelated production change found.
- Concurrency and locks: heartbeat/operator updates use existing volatile/atomic backend state and immutable membership/replica snapshots. New filtering mutates local maps and adds no locks, RPCs, waits or lock-order changes. Existing table snapshots/cache synchronization remain intact. Concrete selected-target failures, rather than a speculative race, establish the finding.
- Lifecycle: DECOMMISSION pre/post-watermark admission and cloud route resolution are preserved. Per-transaction cached locations pin one distribution; fresh empty maps are rejected before insertion. No new static initialization, ownership cycle or release obligation.
- Configuration: no new config. Existing backend load flags are observed dynamically; supported table/global min_load_replica_num exposes the initial-plan defect.
- Compatibility: no persisted format or FE-BE wire change. Metadata replay keeps normal enumeration. Existing configured replica counts still travel beside shorter destination lists, which is the failure-accounting mismatch.
- Thrift/protobuf IDs: no gensrc changes; cross-branch field-ID checks do not apply.
- Parallel paths: initial insert/stream, dynamic create/replace, dummy locations, changelog intersection, cloud default/cluster/endpoint routes, and both writer implementations were examined. One new initial-plan instance is distinct from the retained dynamic thread.
- Special conditions: the empty-map guard has a concrete failure comment and catches UserException into non-OK RPC status on both paths. Nonempty destinations alone do not ensure failure propagation. Changelog same-disk intersection and companion commit validation prevent a separate partial-index corruption issue.
- Test coverage: inspected every added JUnit test and its fixtures/cleanup. Missing end-to-end cases combine mixed partitions, filtered destinations, actual selected-target failure, and reduced quorum. Shutdown, cloud-overload and cache behavior were traced statically.
- Changed test results: new assertions match the intended direct-selection/empty-map behavior; no regression .out files were added or modified. No test execution is claimed.
- Observability: new errors include table/partition/tablet IDs; planner errors include selected/required counts. Existing log-only quorum warnings do not turn the verified silent-success path into a failure. No separate metric requirement identified.
- Transactions and persistence: no new journal fields or replay protocol. DatabaseTransactionMgr derives involved partitions from successful commit infos, so it cannot validate a wholly omitted partition. No additional persistence/failover defect found in the changed mechanisms.
- Data writes, atomicity and crashes: a selected-target failure can commit another partition while losing all input for the omitted one; this violates data correctness and error-means-failure. No separate new leak/deadlock/cleanup issue found; crash behavior was not exercised.
- FE-BE variable propagation: no new field; total/required replica values retain configured counts while location filtering removes destinations. Writers must account for that difference or enforce successful participation of every touched partition.
- Performance: one extra small linear replica-map scan with constant-time backend lookups; cloud cluster resolution remains hoisted. No material performance defect identified.
- Other core invariants: visible-version, delete-bitmap and BE memory-ownership logic are unchanged. No other independent static-review finding remained after the full-scope and risk sweeps.
All eight changed files and all three initial risk focuses were covered. No additional user focus was supplied. This was a static-only review: no builds, tests, source modifications, runtime reproduction or crash/failover experiments were performed. No unresolved static-review candidate or unreviewed changed-file scope remains; execution validation is the explicit coverage limit.
Review converged after 2 round(s).
Existing P0/P1 findings confirmed for this head: #69554 (comment)
8d9cf2e to
962f5b3
Compare
|
/review |
There was a problem hiding this comment.
Request changes: two P1 correctness defects and one P2 regression-test failure remain at head 962f5b3.
The legacy writer now accounts for selected destinations, but its new initialization check rejects the synthetic one-node bootstrap tablet used by empty automatic-partition tables. Separately, VTabletWriterV2 still tolerates failure of every selected destination after filtering, allowing a touched partition to disappear from an otherwise successful mixed-partition load. The new docker regression also enables the fault before an unpruned baseline INSERT, so that setup statement fails before the intended negative case.
Existing review context was independently revalidated: 4361269076 (metadata enumeration), 4361269082 (cloud graceful routing), 4366121929 (empty dynamic locations), and 4367374261 (legacy selected-destination failure accounting) are addressed for their reported paths. None is carried as an existing blocker or reposted. The new V2 finding concerns a separate implementation, VTabletWriterV2::_create_commit_info, which the prior explicitly legacy IndexChannel finding and its fix do not cover. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and evidence: load-specific filtering excludes shared-nothing load-disabled/shutdown backends while retaining metadata and cloud behavior. The overall goal is incomplete because of the bootstrap and V2 defects. Added tests were inspected; no runtime validation was performed.
- Scope and reuse: changes stay within destination selection, replica failure accounting, dynamic quorum propagation, and tests, reusing isLoadAvailable and getLoadRequiredReplicaNum. No unrelated production change found.
- Concurrency and locks: FE RPC/DDL snapshots retain existing table locks; the writer process thread, periodic sender bthread, send pool and BRPC callbacks retain existing _stop_check_channel, _closed_lock and _fail_lock relationships. No new lock, blocking operation under a new lock, or introduced lock-order issue was substantiated. Existing callback/map mutation patterns predate this diff.
- Lifecycle and initialization: count entries precede real-tablet channel initialization; partial initialization still cancels through existing ownership paths. Synthetic tablet lifetime is mishandled by the new quorum check (first P1). No new ownership cycle or cross-translation-unit static initializer was introduced.
- Configuration: no new configuration. Existing disable_load/shutdown flags are read when calculating new destinations; transaction caches intentionally preserve established routing. Existing minimum-load-quorum settings are used in dynamic snapshots.
- Compatibility: no persisted format change; dynamic counts use existing optional protocol fields and BE fallbacks. The BE helper/struct changes are internal. Old writer binaries retain their previous failure-accounting limits until upgraded.
- Thrift/protobuf IDs: no gensrc definition, field ID, enum value, or requiredness change; release-branch ID comparisons are not applicable.
- Parallel paths: initial/create/replace/cache, ordinary/changelog, cloud endpoint/cluster/default selection, and legacy/V2 consumers were traced. FE routing is consistent; V2 final failure accounting is the verified missing parallel fix. The removed single-replica mode is explicitly rejected upstream.
- Conditions and comments: the new real-tablet short-quorum and selected-failure checks explain concrete data-loss scenarios; cloud comments explain why resolved targets are preserved. The checks omit the synthetic bootstrap exception. Decommission replica pre/post-watermark rules remain intact.
- Test coverage: BE tests cover selected-count arithmetic, repeated failures, and short quorum; FE tests cover metadata, filtering, cloud routing, and both dynamic empty-location guards. Missing first-load bootstrap and V2 late-close coverage allowed the P1 defects. The docker regression has the inline P2 setup failure. The planner test helper also leaves partition.getId() unstubbed, so its positive routing tests use mocked quorum zero rather than their intended quorum; this is a coverage limitation, not an additional production finding.
- Test results: no golden output files changed. Aggregate expectations in the docker test are arithmetically consistent if its intended loads execute, but setup must be corrected before it proves those results.
- Observability: new initialization errors identify tablet, partition, load, selected and required counts; dynamic errors identify table/partition/tablet. Existing RPC/failure logs remain sufficient for these paths; no new per-row logging or additional metric requirement found.
- Persistence and failover: transient plan/location metadata changes do not add catalog state or require EditLog changes. Preserving the metadata selector removes the prior replay-divergence concern; commit/publish persistence paths are otherwise unchanged.
- Writes and atomicity: the reported legacy omitted-partition trigger is fixed, but V2 can still return successful commit infos for only the healthy partition after a late receiver tablet failure. FE infers touched partitions from those infos, so the second P1 is an atomicity/data-loss defect. No separate new recovery leak or deadlock was substantiated.
- FE/BE propagation: both dynamic RPCs now serialize effective total/required counts, and original response partitions are parsed for legacy and V2 incremental writers. Selected counts come from existing locations. No new wire variable or definition was added.
- Performance and memory: one additional linear pass over each local tablet's small replica map and one integer per legacy replica-info entry; failure arithmetic remains constant-time. No new large buffer, column/null handling, memory reservation requirement, or cloud per-tablet RPC was introduced. Existing tracking/ownership remains in use.
- Final recheck: all 12 changed paths, existing threads, candidate dispositions, bootstrap paths, unused planned partitions, V2 receiver close errors, cache/retry, old-FE inputs, version gaps and cloud overloads were covered. The three inline findings are the complete accepted new set.
Review scope is static only: no builds, tests, fault injection, source edits, or binary ABI audits were run. Runtime behavior and scheduling were inferred from the cited local control flow. Dedicated end-to-end tests for several traced dynamic/cloud/cache/changelog combinations are absent. No unresolved review candidate or unreviewed assigned path remains.
Review converged after 2 round(s).
962f5b3 to
1298c15
Compare
|
/review |
There was a problem hiding this comment.
Request changes: three new verified issues remain—one legacy fatal assertion on a nonlocal automatic-partition tablet error, one V2 rejection of a quorum-successful shared load, and one older-FE/new-BE minimum-replica compatibility regression. Each is inline. Reviewed all 15 changed files and the relevant upstream/downstream paths statically at head 1298c15.
Critical checkpoint conclusions:
- Goal and proof: load-only eligibility, empty-location rejection and selected-destination failure budgets address the earlier omission scenarios on updated FE/BE binaries. The new findings mean the implementation is not yet complete; added tests were inspected, not executed.
- Scope and reuse: focused selectors, cloud wrappers and shared RPC snapshot/helper changes; no unrelated production changes identified.
- Concurrency: independent sink instances and final RPC callbacks can report tablets beyond local metadata (findings 1–2). Legacy failure updates use
_fail_lock; no additional lock-order inversion or heavy locked operation was identified in the changes. - Lifecycle/static initialization: dummy tablet 0 now uses one destination and avoids real-tablet failure accounting. Shared final-result ownership still conflicts with private metadata. No new ownership cycle, thread or static initialization dependency; the new namespace constant is constexpr.
- Configuration: none added. Existing load-disabled/shutdown flags affect uncached shared-nothing destinations promptly; transaction-cached routes intentionally remain stable.
- Compatibility: existing wire/storage formats are reused, but omitted requirements from older FE dynamic responses cause finding 3. Older BE binaries also retain the historical selected-failure behavior until upgraded; the fixes do not retrofit those binaries.
- Thrift/Protobuf IDs: no gensrc definitions changed; no field-ID/enum additions to audit across branches.
- Parallel paths: initial and incremental legacy/V2 writers, automatic create/replacement RPCs, changelog intersection and cloud wrappers were traced. Removed single-replica write mode is explicitly unsupported.
- Conditions and errors: new checks have concrete failure explanations and identifier-rich messages. The local-ownership assertion and omitted-field inference are invalid in the reported cases; ordinary returned Status failures otherwise propagate.
- Test coverage: inspected FE selector/cloud/RPC tests, BE selected-failure/dummy tests and both regression writer modes. Missing cases include multi-instance incremental late errors and older-FE omitted requirements. The FE location-test mock also leaves partition ID unstubbed, so its declared quorum stub is unused.
- Test results: no result files changed. No builds, tests, cluster runs or runtime validation were performed under the static-only contract.
- Observability: new errors include tablet/partition/load identifiers; no additional metric need was identified for these bounded metadata checks.
- Transactions and persistence: FE still infers touched partitions from successful commit infos, making selected-failure enforcement necessary. No new EditLog/persistence representation or failover protocol was added; request/cache cleanup follows existing paths.
- Data writes and core invariants: normal selected-target failure accounting closes the earlier atomicity hole on updated binaries, but the new callback/owner cases can fatally fail or reject viable writes. Read-visible versions and delete-bitmap version handling are unchanged; bitmap failures were traced as a callback trigger.
- FE/BE variables: existing total/required replica fields are now set by both dynamic RPC paths as well as initial plans; missing old-peer values are the compatibility defect. No new transmitted variable was introduced.
- Memory and performance: only small per-tablet count metadata is added under existing tracking contexts; no new long-lived allocation owner or material asymptotic regression identified. Eligibility is O(replica entries); V2 gap intersection is bounded by replica count.
- Other risks: unselected legacy gap IDs only affect selected-node quorum counting; V2 correctly intersects gap IDs before unioning failures. No additional verified issue from the final risk sweep.
Existing-thread disposition: metadata isolation (4361269076), cloud drain routing (4361269082), empty dynamic locations (4366121929), legacy selected-failure allowance (4367374261), bootstrap tablet (4369805659), V2 selected-failure allowance (4369805662), and regression seed ordering (4369805666) are addressed on the reviewed head. None is reposted; no existing P0/P1 is confirmed still blocking on the updated binaries.
User focus: none supplied. Coverage is complete for the requested static review; runtime behavior remains unexecuted. Review converged after two rounds: all assigned reviewers returned NO_NEW_VALUABLE_FINDINGS on this unchanged payload, all candidates were dispositioned, and all initial risk items were resolved. Convergence describes review coverage, not approval of the code.
Review converged after 2 round(s).
1298c15 to
6f58261
Compare
|
/review |
There was a problem hiding this comment.
Static review of #69554 at 6f58261. All 17 changed paths and relevant upstream/downstream flows were inspected. No builds or tests were run, and no repository source was modified.
One new P1 requires changes: excluding synthetic tablet 0 from the legacy writer's backend map can discard a real tablet's final-close error when an empty-input sender closes the bootstrap receiver last. A second partition can commit while every row in the failed partition is omitted. The inline finding describes the concrete trigger and fix direction.
The original P1 triggers in comments 4361269076, 4366121929, 4367374261, 4369805659, 4369805662 and 4370332388 are addressed on this head: metadata enumeration is separate from load selection; both dynamic RPCs reject empty locations; both writers account for selected destinations; bootstrap policy is 1/1/1; and nonlocal errors no longer reach the missing-policy assertion. No existing P0/P1 ID is carried forward. The new dummy-only error bypass is a distinct changed location and silent-loss outcome.
Known residuals remain under their existing threads and are not reposted: 4370332388 still covers quorum-valid legacy loads rejected by the guarded nonlocal path; 4370332390 is fixed for local pooled sinks but foreign-source V2 results can still lack policy; 4370332391 fixes selected=1 but an older FE's omitted minimum still causes selected=2/minimum=1 loads to reject one late failure. The cloud routing issue 4361269082 and regression seeding issue 4369805666 are addressed.
Critical checkpoint conclusions:
| Checkpoint | Conclusion |
|---|---|
| Goal and evidence | New shared-nothing loads filter load-disabled/shutdown backends while metadata/cloud routing is preserved. Atomicity remains incomplete because of the new P1. Added tests were inspected, not executed. |
| Scope, clarity and reuse | Changes stay focused on load selection, replica policy and failure handling; dynamic RPCs reuse one emptiness guard. |
| Concurrency and locks | Pool metadata uses the existing mutex; failure status uses the existing failure lock. No new lock inversion or blocking I/O under the policy lock was found. Cross-sender final-EOS result ownership causes the P1. |
| Lifecycle and static initialization | Empty-input writers open and send EOS. Receiver results can include other senders' tablets. Shared pool policy lifetime follows the existing shared owner; constexpr IDs add no static initialization dependency. |
| Configuration | No configuration item is added. Fresh selection observes existing flags; in-flight/cached locations retain transaction stability. |
| Compatibility | Existing optional policy fields are consumed and omitted fields have fallback handling in both writers. The known selected=2 older-FE minimum-policy residual remains as described above. No storage format is changed. |
| Thrift/protobuf IDs | No definition or field ID is changed; release-branch ID comparison is not applicable. |
| Parallel paths | Both initial plan builders, create/replace-partition RPCs, cloud overloads, changelog colocation, legacy and V2 writers were inspected. The new P1 is specific to legacy bootstrap-only bookkeeping. |
| Special conditions | Insufficient-selection checks and cloud bypass have concrete explanations. The tablet-0 exclusion bypasses explicit nonlocal error handling, as documented in the inline finding. |
| Test coverage | FE tests cover metadata/load separation, cloud preservation and dynamic empty rejection. BE tests cover policy arithmetic, bootstrap and local pooled policy; the regression covers mixed-partition selected-target failures. They omit an empty-input final sender, foreign-source policy ownership and the older-FE selected=2 case. |
| Test results | No expected-result artifact is added or modified. No execution result or passing test is claimed. |
| Observability | New selection/policy errors include relevant tablet, partition, backend or load identifiers. No separate logging/metrics defect was established; the P1 silently discards an error before it becomes failure status. |
| Transaction, persistence and failover | No EditLog format or transaction state transition changes. FE still derives involved partitions from successful commit infos, which makes the discarded final error capable of omitting a whole partition. Existing replay metadata behavior is preserved. |
| Writes, atomicity and crashes | Ordinary selected-target failure budgets are corrected in both writers, and the original callback assertion crash is removed. The new P1 can still report partial data as a successful transaction. No additional crash/leak was substantiated. |
| FE-BE propagation | Both initial senders and both dynamic RPCs populate existing total/required fields; both consumers were traced. V2 policy is registered before stream opening. Known cross-source ownership limits are described above. |
| Performance and memory | Added policy state is linear in tablets and replica-gap entries, with bounded replica-set copies and no new unbounded lifetime. No new hub-header include, PCH/template/unity change or libc API was introduced. |
| Other issues and user focus | No additional user focus was supplied. Final missed-issue checks included empty-input startup, both EOS stages, sender counts, actual fault strings, cached locations, decommission watermarks and version gaps. No other new verified finding remains. |
Verification limits: this was static inspection only; compilation, runtime fault reproduction and test execution remain unverified. Known issues above have explicit duplicate dispositions and are distinct from unresolved review evidence.
Review converged after 2 round(s).
| if (tablet.tablet_id != V1_DUMMY_TABLET_ID) [[likely]] { | ||
| // Tablet 0 only bootstraps an empty automatic table. It receives no rows and must | ||
| // not consume the real tablets' failure allowance before partitions are created. | ||
| _tablets_by_channel[replica_node_id].insert(tablet.tablet_id); |
There was a problem hiding this comment.
[P1] Preserve real tablet errors for bootstrap-only senders
In a parallel legacy load into an empty automatic table, W2 can open only tablet 0 on BE1 while W1 creates and writes real partitions. With replication=3 and min_load_replica_num=1, let A have only BE1 selected because its other two replicas are load-disabled, and let B have healthy destinations elsewhere. Excluding tablet 0 here leaves W2 without any _tablets_by_channel[BE1] entry. If W1 sends EOS first and W2 closes BE1 last, TabletsChannel::close() returns A's late rowset-build/delete-bitmap/commit error only to W2. Its mark_as_failed() returns at the missing-backend lookup before reaching the new nonlocal-tablet guard. Neither writer reports A's failure or an A commit info, while B's commit infos let FE skip A entirely; the load can succeed with all A rows lost. Retain backend bookkeeping while excluding the dummy tablet from failure budgets, or handle explicit tablet errors before that early return, and cover an empty-input final sender in a mixed-partition regression. Unlike thread 4370332388's sender with real-tablet bookkeeping, this new dummy-only branch silently discards the error instead of rejecting the load.
In a three-replica deployment, marking a slow backend with disable_load did not remove it from tablet load locations, so writers still sent data to that backend and suffered long-tail latency. The disable_load feature originally checked backend load availability, but the single-replica migration fix later changed tablet load selection to check only whether a backend was alive. Restore the load-availability check while preserving the decommission watermark rule that allows an in-flight replica migration to continue writing until its post watermark is set.