[chore](load) Remove the unused single replica load feature - #66623
Conversation
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: Update DeltaWriter callers to require the simplified transaction commit API that remains after removing the unused single replica load implementation. This is the compile-time RED checkpoint: the focused test translation unit fails because the production API still requires PSlaveTabletNodes.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- Compile-time RED: delta_writer_test.cpp fails with the expected missing no-argument commit_txn overload
- Behavior changed: No
- Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
556bdcb to
6d1affd
Compare
|
run buildall |
Issue Number: N/A
Related PR: N/A
Problem Summary: The unused single replica load path duplicated FE planning, tablet location, BE write, transaction callback, rowset pull, and HTTP download logic. Remove the runtime configuration and session switches together with the master/slave execution paths and dedicated tests. Keep the Thrift and protobuf contracts unchanged; retained RPC methods and old write_single_replica requests now fail explicitly with NotSupported. Ordinary loads always send data to all selected replica locations.
Remove the experimental single replica load and single replica insert options.
- Test:
- Unit Test: ./run-fe-ut.sh --run org.apache.doris.planner.OlapTableSinkTest,org.apache.doris.planner.GroupCommitBlockSinkTest,org.apache.doris.transaction.AutoPartitionCacheManagerTest (7 tests passed)
- Manual test: compiled the changed BE translation units and affected DeltaWriter/VRowDistribution test translation units with be/ut_build_ASAN/compile_commands.json
- BE unit-test linking was attempted but blocked by the missing thirdparty/installed/lib64/libarrow_compute.a in this worktree
- Behavior changed: Yes. Single replica load settings are removed and retained protocol requests are rejected explicitly.
- Does this need documentation: No. The removed switches belonged to an unused experimental feature.
[fix](fe) Fix Nereids checkstyle violations
Issue Number: None
Related PR: None
Problem Summary: FE Checkstyle validation failed because RemoteOlapInsertExecutor had an extra blank line before its class Javadoc and NereidsLoadPlanInfoCollector retained an unused Config import. Remove both violations so FE style validation succeeds.
None
- Test: No need to test (style-only import and whitespace cleanup)
- Check: `mvn checkstyle:check -pl fe-core`
- Behavior changed: No
- Does this need documentation: No
6d1affd to
0eaefa1
Compare
|
run buildall |
TPC-H: Total hot run time: 29410 ms |
TPC-DS: Total hot run time: 158499 ms |
ClickBench: Total hot run time: 23.97 s |
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
FE Regression Coverage ReportIncrement line coverage |
|
run feut |
FE UT Coverage ReportIncrement line coverage |
|
skip check_coverage |
|
/review |
There was a problem hiding this comment.
I found three blocking compatibility regressions in the single-replica-load removal: an old master can wait until roughly 90% of the load timeout when an upgraded slave returns an application error without the legacy completion callback; an upgraded selected master can silently commit only its local replica while ignoring an old sender's slave metadata; and the removed session variable is not registered in Doris's removed-variable compatibility set.
Checkpoint conclusions:
- Goal, scope, and proof: the all-new FE/BE path is consistently simplified to normal full-replica writes, but the change does not safely accomplish removal across retained rolling-upgrade and client-compatibility boundaries. Current tests do not prove those boundaries.
- Size and focus: the cleanup is mechanically focused and removes the feature-owned state together, but the retained protocol stubs and public variable surface need the compatibility fixes called out inline.
- Concurrency and lifecycle: the feature-owned callback maps, locks, waits, timers, worker, transaction-writer registry, and endpoint are otherwise removed consistently; ordinary writer and download lifecycles remain intact. The legacy callback lifecycle in the old-master/new-slave placement is the first inline defect. No new lock-order, static-initialization, or ownership issue was found.
- Configuration: no configuration is added. Stale FE/BE config-file keys are warned and ignored, and removed persisted/forwarded session fields are skipped, but explicit legacy
SET/SET GLOBAL/SELECT @@access is broken as described inline. - Compatibility and FE/BE passing: retained Thrift/protobuf field IDs and default-false behavior let new senders use old receivers normally. Two old-request/new-BE placements are unsafe: the callback path and receiver-side add-block path described inline. No new cross-layer variable was added.
- Parallel paths and conditions: local/remote INSERT, Nereids load collection, group commit, initial locations, automatic create, overwrite replace, retry cache, adaptive bucket, and load-to-single-tablet paths all propagate the full selected replica set where consumed. The new sink-executor rejection guard is valid but cannot protect an upgraded receiving tablet master.
- Error handling: the retained slave-pull RPC's application status is ignored by the old caller, while the upgraded selected master returns a misleading OK after discarding slave metadata. Other changed status/error paths are checked consistently.
- Tests and results: the modified unit tests are consistent with the new ordinary API shape and cache behavior, and current CI checks are green. There is no mixed-version coverage for either failing placement and no removed-variable compatibility test; the deleted feature-specific suites do not cover those gaps.
- Transactions, persistence, and data correctness: FE quorum enforcement prevents the under-replicated local write from becoming a successful visible transaction, but affected loads fail late or after avoidable work. No EditLog, replay, failover, MoW, visible-version, or ordinary atomicity defect was found.
- Observability: existing identifiers are adequate for ordinary paths, but returning OK after ignoring legacy slave metadata hides the actual incompatibility; prompt non-OK rejection would make it diagnosable. No additional metric is otherwise needed.
- Performance: the all-new path removes replica-pull overhead, but the old-master/new-slave placement can stall for about 90% of the configured load timeout. No other CPU, memory, or complexity regression was found.
- Memory and nullable handling: no new allocation ownership, reservation, COW, nullable-column, or const-nullable behavior is introduced by this removal.
- Other issues: no additional valuable finding survived the complete second pass. No additional user-provided review focus was specified.
Review completion: converged after 2 rounds; all candidates were independently verified and deduplicated; 3 inline comments are included.
ignore,
These issues are only reachable if single replica load was enabled on an old version or clients still access its experimental session variable. This experimental feature has never been used in production and is being removed intentionally, so these compatibility scenarios
are out of scope and should not block this PR.
Single replica load is an experimental, disabled-by-default load path that writes data to one master replica first and then pulls the generated rowset to slave replicas.
This feature maintains a separate end-to-end implementation across FE and BE, including:
The feature is no longer actively used, while its parallel execution path adds considerable maintenance cost and introduces additional transaction, callback, concurrency, and upgrade complexity.
This PR removes the single replica load implementation and makes normal load paths always write to all selected replicas.
The following options are removed:
enable_single_replica_loadenable_single_replica_loadenable_single_replica_insertThe Thrift and protobuf definitions are intentionally kept unchanged to avoid protocol schema and generated-code churn.
For compatibility and data safety:
write_single_replica = truereturnsNotSupportedexplicitly instead of silently writing only one replica.NotSupported.write_single_replica,slave_location, or slave tablet locations.Feature-specific regression cases are removed, while existing normal load tests are updated to use the simplified
DeltaWriter::commit_txn()interface.None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)