Skip to content

[fix](cloud) Fix rowset notification cleanup and update cloud load defaults - #70218

Merged
bobhan1 merged 3 commits into
apache:masterfrom
bobhan1:fix/cloud-committed-rowset-cleanup
Nov 10, 2026
Merged

bobhan1 merged 3 commits into
apache:masterfrom
bobhan1:fix/cloud-committed-rowset-cleanup

Conversation

@bobhan1

@bobhan1 bobhan1 commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Background

After a cloud load transaction commits in Meta Service, FE can notify the participating BEs of its visible versions. BE uses cached committed rowsets or lightweight empty-rowset entries to update local tablet metadata, with normal metadata synchronization as the fallback.

What problem does this PR solve?

Issue Number: None
Related PR: #59754 (introduced the notification path), #70209 (fixes the unrelated Parquet unit-test build failure and is now included in master)
PR Type: Bugfix

Problem Summary: Completed notification tasks retain their registered signatures, including when the BE feature is disabled. Consumed empty-rowset markers also remain cached because removal only covers the separate non-empty-rowset map. Release signatures on every callback exit and represent both rowset kinds in one cache so removal and expiration share the same lifecycle. Enable the existing FE and BE notification settings by default, and default cloud_mow_sync_rowsets_when_load_txn_begin to false so cloud MoW loads skip forced synchronization when they begin. Remove the six now-redundant FE/BE overrides from the Cloud p0/p1 pipeline configs so those pipelines exercise the new defaults with unchanged effective settings.

Details

  • Frequency: Every accepted notification leaves its signature behind; every consumed empty entry remains until expiration.
  • Root cause: The callback does not pair task registration with removal, and the split empty/non-empty caches have different removal behavior.
  • Why existing tests missed it: They cover insertion and expiration but do not check removal of an empty entry or resubmission after a notification callback completes.
  • Fix: Add a Defer guard before the BE enable check. Store empty rowsets as null metadata in the existing committed-rowset cache, reuse insertion and expiration, and avoid a redundant lookup during cleanup.
  • Other paths and branches: The guard covers enabled notifications, disabled notifications and cache misses. Non-empty and empty committed rowsets now share cleanup; the independent MoW delete-bitmap cache is unchanged. This change targets master.
  • Behavior change: enable_notify_be_after_load_txn_commit and enable_cloud_make_rs_visible_on_be now default to true; cloud_mow_sync_rowsets_when_load_txn_begin now defaults to false. All three settings retain their existing dynamic mutability. The cloud rowset-builder test fixture now restores the incoming setting rather than hard-coding true at teardown.
  • Concurrency and lifecycle: Cache operations retain the existing shared mutex and acquire no new nested locks. Signature cleanup runs after tablet processing has released its locks. Refreshed entries remain protected from older expiration records.
  • Compatibility and data correctness: RPC fields and persistent formats are unchanged. Cloud MoW loads continue to use a local flush snapshot. Skipping initialization synchronization enlarges the existing snapshot freshness window; the partial-update validation and publish-alignment algorithms are unchanged. The rolling-upgrade caveat is described below.
  • Observability and cost: Move per-rowset insertion logging outside the cache lock and lower it to DEBUG as notifications become enabled by default. Existing notification and expiration logs retain transaction/tablet identifiers.

Testing

  • The implementation commits were rebased onto master 87b462e4f58104140f098f5f4a8b5d58bec70149. The BE/FE runs below tested implementation head 17d42f6ef184c7f851920830ad3ce4861fe679aa. Final head 4ba6d0db1480dd018a2e9353cffbb6137fe1c65d adds only the pipeline-configuration cleanup.
  • On the rebased head, ./run-be-ut.sh --run --filter='CloudCommittedRSMgrTest.*:EnabledAndDisabled/CloudCommittedRowsetTaskTest.*:TaskWorkerPoolTest.*:CloudTabletApplyVisiblePendingTest.*:CloudGroupRowsetBuilderWriterTest.*:*DeltaWriterCancelIntegrationTest.*' -j100 passed all 46 ASAN tests with the requested defaults.
  • On the same rebased head, ./run-fe-ut.sh --run org.apache.doris.cloud.transaction.CloudGlobalTransactionMgrTest passed all 56 tests (0 failures, errors or skips); the Maven reactor completed successfully.
  • The BE build compiled both task_worker_pool_test.cpp and vparquet_writer_test.cpp successfully. The latter uses the [fix](test) Build vparquet_writer_test against the location-based Iceberg WriteInfo #70209 fix already merged into master; no temporary source patch was applied.
  • Before the original cleanup fix, the empty-entry removal test and both enabled/disabled signature-release tests failed; the expiration-refresh test passed.
  • build-support/check-build-hygiene.sh and git diff --check passed after rebase. The unchanged PR patch previously passed clang-format v16/check-format and FE fe-common Maven validate/Checkstyle.
  • For the final pipeline cleanup, checked all six removed values against the FE/BE defaults, compared the remaining parsed settings in all four files, and ran git diff --check: all passed. BE/FE tests were not rerun for this config-only commit because its effective pipeline values are unchanged.
  • clang-tidy was not rerun, as requested. A cloud end-to-end regression and a standalone production BE build were not run. The snapshot-window and mixed-version behavior below was assessed from code, without a running-cluster reproduction.

Notes for Reviewers

Local review of 4ba6d0db1480dd018a2e9353cffbb6137fe1c65d found no Blocker or Major issue. One non-blocking P2 remains for rolling upgrades.

Existing review finding Current disposition
MF-001: FE notification retries False positive: this notification is not inserted into AgentTaskQueue.
MF-002: stale MoW snapshot Documented behavior tradeoff of the existing mode; the new-P1 classification is withdrawn under the intended load semantics.
MF-003: old-BE signature leak P2 remains for a new-FE/old-BE rolling-upgrade window.
MF-004: protected test access False positive: BE UT uses -fno-access-control, and the tests compiled and passed.

Snapshot semantics: Even with initialization synchronization enabled, sync_rowsets, local snapshot construction and the later flush are not atomic with other transactions' commits. A key absent from the captured snapshot can already enter the new-key validation path and fail under ERROR or for an omitted non-nullable column without a default. Disabling the initial synchronization can enlarge the freshness window, including while an earlier MS-committed rowset is still absent locally. This PR deliberately makes that existing mode the default; master already configures Cloud p0/p1 to use it. This is an explicit behavior/cost tradeoff, not a claim that local snapshots always contain the latest committed keys. For APPEND updates that reach publication, delete-bitmap calculation synchronizes missing versions and realigns conflicting rows from historical columns; flush-time default/null fill alone does not establish final data loss.

Rolling upgrades: Enabling the new FE notification default while an old BE supports this task but lacks the cleanup guard exposes the old BE signature leak, including when its local notification setting is false. That process retains the signatures until it is restarted/upgraded. Keep FE notifications disabled until all receiving BEs include the cleanup fix.

Release note

Cloud committed-rowset notifications are enabled by default through enable_notify_be_after_load_txn_commit on FE and enable_cloud_make_rs_visible_on_be on BE. Completed notifications release their task signatures, and consumed empty-rowset entries are removed promptly. The BE setting cloud_mow_sync_rowsets_when_load_txn_begin now defaults to false, so cloud MoW loads skip forced rowset synchronization at initialization unless it is explicitly enabled.

Check List (For Author)

  • Behavior changed: Yes (see Details).
  • Does this need documentation: Yes; document all three default-value changes in the configuration reference and release notes.
  • Pre-submission status: Partially verified: the implementation passed the targeted BE/FE tests and the final pipeline cleanup passed static effective-configuration checks. Cloud end-to-end regression and a standalone production BE build were not run; clang-tidy was excluded as requested. The local review has no Blocker/Major finding, with the rolling-upgrade P2 and snapshot behavior documented above.

AI Disclosure

@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

bobhan1 added a commit to bobhan1/doris that referenced this pull request Nov 4, 2026
### Background

Cloud MoW load initialization can synchronize rowsets from Meta Service before capturing its local rowset snapshot. The existing mutable cloud_mow_sync_rowsets_when_load_txn_begin setting controls this synchronization.

### What problem does this PR solve?

Issue Number: None
Related PR: apache#59754 (introduced the setting), apache#70218 (enables notifications and fixes cleanup), apache#70209 (temporary BE UT build prerequisite)
PR Type: Enhancement

Problem Summary: Make rowset synchronization at the start of a cloud MoW load opt-in by changing cloud_mow_sync_rowsets_when_load_txn_begin from true to false. Adjust the related CloudGroupRowsetBuilderWriterTest fixture to restore the incoming configuration value instead of hard-coding true during teardown.

### Details

- **Before and after:** Cloud MoW loads no longer force sync_rowsets before constructing their initial local rowset snapshot by default.
- **Configs and defaults:** cloud_mow_sync_rowsets_when_load_txn_begin changes from true to false and remains dynamically mutable.
- **Impact on existing users:** An explicit true setting retains synchronization at load initialization. Notification delivery and subsequent metadata synchronization remain relevant to local rowset freshness.
- **Behavior change:** Yes; skip the initial synchronization unless the setting is enabled. The test fixture preserves whichever configuration value it received.

### Testing

- ./run-be-ut.sh --run --filter='CloudGroupRowsetBuilderWriterTest.*:*DeltaWriterCancelIntegrationTest.*' -j100 passed all 12 ASAN tests after this change.
- The unit-test build temporarily applied apache#70209 to fix the unrelated vparquet_writer_test.cpp compilation failure. That prerequisite was removed afterward and is not included in this commit.
- clang-format v16/check-format on both changed files and git diff --check passed.
- The earlier 36 BE and 56 FE passing tests belong to commit 1b4d168, before this default update; they were not rerun for this commit.
- clang-tidy was omitted. The stale-cache partial-update scenario below was not exercised in a running cloud cluster.

### Notes for Reviewers

The stale-rowset partial-update issue identified during source review remains unresolved: if a BE has not received or synchronized a prior committed rowset, constructing the MoW context from its local snapshot can classify an existing key as new and fail a partial update under the ERROR policy or when an omitted column is non-nullable and has no default. This commit changes the default; it does not repair that path.

### Release note

The BE setting cloud_mow_sync_rowsets_when_load_txn_begin now defaults to false. Cloud MoW loads skip forced rowset synchronization at initialization unless this setting is explicitly enabled.

### Check List (For Author)

- Test: Unit Test; 12 BE ASAN tests passed with the prerequisite described above.
- Behavior changed: Yes (see Details).
- Does this need documentation: Yes; include this third default-value change in the configuration reference and release notes alongside the two notification settings.
- Pre-submission status: Partially verified: focused tests passed; the known stale-cache partial-update risk remains unresolved and cloud end-to-end validation is pending.

### AI Disclosure

- Tool: Codex (GPT-6).
- Generated: Code, test fixture adjustment and this description.
- Human review: Pending.
@bobhan1 bobhan1 changed the title [fix](cloud) Clean up committed rowset notifications and enable them by default [fix](cloud) Fix rowset notification cleanup and update cloud load defaults Nov 4, 2026
@bobhan1
bobhan1 marked this pull request as ready for review November 5, 2026 02:54
@bobhan1

bobhan1 commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

run buildall

@bobhan1

bobhan1 commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

/review

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 28851 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 3debacfd682b4b6abcf6b147dfe949b9cf18cf28, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17606	5171	5091	5091
q2	2157	297	252	252
q3	10242	1392	820	820
q4	4677	465	341	341
q5	7430	800	533	533
q6	206	190	155	155
q7	770	807	601	601
q8	9264	1360	1078	1078
q9	5536	4223	4197	4197
q10	6808	1294	1025	1025
q11	417	247	233	233
q12	637	397	282	282
q13	18127	3712	3087	3087
q14	273	268	252	252
q15	q16	742	710	671	671
q17	1184	744	574	574
q18	7107	6160	6096	6096
q19	1260	955	607	607
q20	382	339	223	223
q21	6315	2547	2444	2444
q22	380	343	289	289
Total cold run time: 101520 ms
Total hot run time: 28851 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	5932	6000	6134	6000
q2	786	624	591	591
q3	5656	5972	5306	5306
q4	2035	2183	1548	1548
q5	5778	5877	5599	5599
q6	254	208	157	157
q7	2301	1998	1831	1831
q8	3081	2674	2743	2674
q9	7971	7444	7438	7438
q10	3679	3605	3172	3172
q11	591	443	417	417
q12	682	683	520	520
q13	3383	3634	3050	3050
q14	292	291	269	269
q15	q16	670	702	644	644
q17	7924	7112	6975	6975
q18	12912	12220	12845	12220
q19	902	784	790	784
q20	2147	2145	1917	1917
q21	5833	4777	4958	4777
q22	508	489	433	433
Total cold run time: 73317 ms
Total hot run time: 66322 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 151390 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 3debacfd682b4b6abcf6b147dfe949b9cf18cf28, data reload: false

query5	4311	594	440	440
query6	424	208	190	190
query7	4814	447	230	230
query8	314	166	152	152
query9	8823	3167	3162	3162
query10	475	318	260	260
query11	5862	3870	3573	3573
query12	150	92	86	86
query13	1253	443	321	321
query14	6129	3934	3679	3679
query14_1	3423	3386	3410	3386
query15	205	209	185	185
query16	948	441	433	433
query17	849	657	547	547
query18	2415	424	314	314
query19	202	163	144	144
query20	87	84	83	83
query21	211	157	107	107
query22	12925	12986	12868	12868
query23	13462	12852	12277	12277
query23_1	12580	12563	12384	12384
query24	7365	822	473	473
query24_1	462	482	470	470
query25	510	408	341	341
query26	1260	261	139	139
query27	2757	436	271	271
query28	4641	2016	2037	2016
query29	1553	579	429	429
query30	286	216	185	185
query31	990	925	826	826
query32	145	88	85	85
query33	530	322	228	228
query34	880	858	494	494
query35	729	751	643	643
query36	799	810	725	725
query37	132	97	85	85
query38	1779	1734	1671	1671
query39	794	809	743	743
query39_1	729	730	731	730
query40	208	114	92	92
query41	66	63	62	62
query42	88	81	80	80
query43	342	359	315	315
query44	1337	701	704	701
query45	177	179	166	166
query46	836	955	566	566
query47	2809	3084	2784	2784
query48	295	320	219	219
query49	567	419	293	293
query50	682	270	207	207
query51	10475	10730	10588	10588
query52	78	77	69	69
query53	182	207	149	149
query54	234	193	182	182
query55	75	72	67	67
query56	250	215	220	215
query57	1665	1600	1617	1600
query58	296	259	289	259
query59	2140	2144	2014	2014
query60	283	235	225	225
query61	147	141	141	141
query62	381	345	284	284
query63	186	157	157	157
query64	2720	940	745	745
query65	3348	3305	3273	3273
query66	1781	412	294	294
query67	20027	20178	20155	20155
query68	2943	1018	615	615
query69	387	285	245	245
query70	824	761	764	761
query71	290	228	205	205
query72	2674	2611	2409	2409
query73	512	536	312	312
query74	4618	4533	4357	4357
query75	2348	2307	1928	1928
query76	2315	1018	590	590
query77	359	379	268	268
query78	9888	9683	9146	9146
query79	1011	860	543	543
query80	1186	442	383	383
query81	591	329	272	272
query82	582	139	114	114
query83	312	202	187	187
query84	321	126	116	116
query85	937	499	364	364
query86	388	237	229	229
query87	2033	1953	1786	1786
query88	3768	2799	2771	2771
query89	328	281	240	240
query90	1859	166	163	163
query91	155	150	127	127
query92	87	89	84	84
query93	1013	1006	578	578
query94	593	300	266	266
query95	581	392	293	293
query96	666	483	240	240
query97	2433	2426	2319	2319
query98	161	155	158	155
query99	759	768	650	650
Total cold run time: 234516 ms
Total hot run time: 151390 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 24.35 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 3debacfd682b4b6abcf6b147dfe949b9cf18cf28, data reload: false

query1	0.01	0.01	0.00
query2	0.10	0.04	0.04
query3	0.26	0.09	0.08
query4	1.61	0.12	0.09
query5	0.27	0.26	0.25
query6	1.17	0.69	0.68
query7	0.04	0.01	0.01
query8	0.05	0.04	0.03
query9	0.40	0.34	0.36
query10	0.59	0.59	0.58
query11	0.20	0.15	0.14
query12	0.18	0.15	0.15
query13	0.49	0.49	0.49
query14	1.03	1.00	1.00
query15	0.63	0.62	0.62
query16	0.35	0.36	0.34
query17	1.18	1.18	1.15
query18	0.20	0.17	0.18
query19	2.09	1.97	1.92
query20	0.02	0.02	0.01
query21	15.49	0.19	0.13
query22	4.89	0.05	0.06
query23	16.28	0.33	0.12
query24	2.89	0.51	0.46
query25	0.11	0.06	0.04
query26	1.61	0.21	0.16
query27	0.05	0.05	0.05
query28	3.85	0.81	0.38
query29	12.53	4.20	3.25
query30	0.28	0.13	0.10
query31	2.77	0.58	0.34
query32	3.22	0.62	0.51
query33	3.23	3.18	3.23
query34	15.57	4.03	3.38
query35	3.34	3.31	3.36
query36	0.56	0.44	0.44
query37	0.09	0.06	0.06
query38	0.06	0.04	0.04
query39	0.04	0.03	0.03
query40	0.18	0.15	0.14
query41	0.10	0.03	0.03
query42	0.04	0.03	0.03
query43	0.05	0.04	0.03
Total cold run time: 98.1 s
Total hot run time: 24.35 s

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review of PR 70218 at head 3debacf against base 380912c. The review converged after 3 rounds; all final-round subagents returned NO_NEW_VALUABLE_FINDINGS.

Verified findings:

  • MF-001 (P1): the new BE scope-exit signature removal leaves the FE AgentTaskQueue entry resendable, with no finish or queue-removal path, so normal visibility notifications are resent on every report cycle.
  • MF-002 (P1): disabling the default rowset sync lets cloud MoW partial updates snapshot stale local rowsets after a missed best-effort visibility notification, causing existing keys to be treated as new and potentially failing or default/null-filling rows.
  • MF-003 (P2): the new FE notification default sends tasks to older BEs whose callback still returns before cleanup, retaining one signature per committed transaction during a mixed-version rollout.
  • MF-004 (P1): the new parameterized BE test accesses protected TaskWorkerPool::_thread_pool at both waits, so the test target fails C++ access checking before linking.

Review checkpoints: cloud committed-rowset null-marker semantics, expiration generation checks, lock boundaries, and shared-pointer lifetime were traced and found sound. Callback scope-exit behavior preserves in-flight dedup; the FE resend lifecycle is the accepted MF-001 issue. MOW snapshot freshness, partial-update handling, and FE/BE mixed-version compatibility are covered by MF-002 and MF-003. Changed tests and teardown/config restoration were reviewed; MF-004 is the compile-blocking test issue. No existing inline review threads or comments applied, and the supplied user focus file contained no extra focus. Validation was static inspection only: no builds, tests, benchmarks, or runtime validation were run. No unresolved candidate remains; existing_blocking_comment_ids is empty.

Review converged after 3 round(s).

@@ -2594,6 +2594,7 @@ void calc_delete_bitmap_callback(CloudStorageEngine& engine, const TAgentTaskReq

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Removing this signature makes FE resend the notification forever. sendMakeCloudTmpRsVisibleTasks adds MakeCloudTmpRsVisibleTask to AgentTaskQueue; this type is resendable, and ReportHandler.taskReport resubmits queue entries absent from BE-reported signatures. This callback never sends finish_task or removes the FE queue entry, so the scope-exit cleanup makes each normal notification disappear from BE reports and get resent on every report cycle. Complete/remove the FE task, or make it non-resend, before clearing the BE signature.

Comment thread be/src/common/config.cpp
@@ -2148,9 +2148,9 @@ DEFINE_mInt32(concurrency_stats_dump_interval_ms, "100");
DEFINE_Validator(concurrency_stats_dump_interval_ms,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Defaulting this flag to false lets MoW partial updates snapshot stale local rowsets. CloudRowsetBuilder::init now skips CloudTablet::sync_rowsets() before init_mow_context, which probes only the local snapshot. If a prior commit exists in Meta Service but a best-effort notification was missed (or during a BE-first rollout), an existing key is classified as new; ERROR policy fails the load, while append policy can fill omitted columns with defaults/nulls instead of historical values. Keep synchronization enabled until a freshness fallback is in place, or sync whenever the snapshot is not known current.

@@ -3759,7 +3759,7 @@ public void handle(Field field, String value) throws Exception {
public static boolean enable_tso_forward_when_counter_full = true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Enabling this FE default sends visibility tasks to old BEs whose enable_cloud_make_rs_visible_on_be default is still false. Their callback returns after _submit_task has inserted the signature into s_task_signatures, so every committed transaction leaves a signature in the old BE's set and report payload indefinitely during a rolling upgrade. Gate this default on BE capability/upgrade, or make the old early-return path clean up.

TEST_P(CloudCommittedRowsetTaskTest, ReleasesSignatureAfterCallback) {
const bool original_enabled = config::enable_cloud_make_rs_visible_on_be;
Defer restore_config = [&] { config::enable_cloud_make_rs_visible_on_be = original_enabled; };
config::enable_cloud_make_rs_visible_on_be = GetParam();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] The new test cannot compile because it reaches into a protected member. TaskWorkerPool::_thread_pool is declared under protected: in task_worker_pool.h, but this free TEST_P body calls workers._thread_pool->wait() at both waits. Use a public wait/test helper instead of accessing the member directly.

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage `` 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 100% (0/0) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 77.67% (35915/46241)
Line Coverage 63.03% (404010/641022)
Region Coverage 59.64% (342142/573663)
Branch Coverage 60.78% (158472/260746)

gavinchou
gavinchou previously approved these changes Nov 5, 2026
…by default

### Background

After a cloud load transaction commits in Meta Service, FE can notify the participating BEs of its visible versions. BE uses cached committed rowsets or lightweight empty-rowset entries to update local tablet metadata, with normal metadata synchronization as the fallback.

### What problem does this PR solve?

Issue Number: None
Related PR: apache#59754 (introduced the notification path), apache#70209 (temporary prerequisite for local BE unit-test builds)
PR Type: Bugfix

Problem Summary: Completed notification tasks retain their registered signatures, including when the BE feature is disabled. Consumed empty-rowset markers also remain cached because removal only covers the separate non-empty-rowset map. Release signatures on every callback exit and represent both rowset kinds in one cache so removal and expiration share the same lifecycle. Enable the existing FE and BE notification settings by default.

### Details

- **Frequency:** Every accepted notification leaves its signature behind; every consumed empty entry remains until expiration.
- **Root cause:** The callback does not pair task registration with removal, and the split empty/non-empty caches have different removal behavior.
- **Why existing tests missed it:** They cover insertion and expiration but do not check removal of an empty entry or resubmission after a notification callback completes.
- **Fix:** Add a Defer guard before the BE enable check. Store empty rowsets as null metadata in the existing committed-rowset cache, reuse insertion and expiration, and avoid a redundant lookup during cleanup.
- **Other paths and branches:** The guard covers enabled notifications, disabled notifications and cache misses. Non-empty and empty committed rowsets now share cleanup; the independent MoW delete-bitmap cache is unchanged. This change targets master.
- **Behavior change:** enable_notify_be_after_load_txn_commit and enable_cloud_make_rs_visible_on_be now default to true. Both retain their existing dynamic mutability. cloud_mow_sync_rowsets_when_load_txn_begin remains true.
- **Concurrency and lifecycle:** Cache operations retain the existing shared mutex and acquire no new nested locks. Signature cleanup runs after tablet processing has released its locks. Refreshed entries remain protected from older expiration records.
- **Compatibility and data correctness:** No RPC fields, persistent formats, transaction commit rules or partition-version visibility checks change. Missing cached data continues to use the existing metadata synchronization path.
- **Observability and cost:** Move per-rowset insertion logging outside the cache lock and lower it to DEBUG as notifications become enabled by default. Existing notification and expiration logs retain transaction/tablet identifiers.

### Testing

- Before the fix: four new ASAN cases ran; empty-entry removal and both enabled/disabled signature-release cases failed, while the expiration-refresh case passed.
- After the final change: ./run-be-ut.sh --run --filter='CloudCommittedRSMgrTest.*:EnabledAndDisabled/CloudCommittedRowsetTaskTest.*:TaskWorkerPoolTest.*:CloudTabletApplyVisiblePendingTest.*:CloudGroupRowsetBuilderWriterTest.*' -j100 passed all 36 tests.
- ./run-fe-ut.sh --run org.apache.doris.cloud.transaction.CloudGlobalTransactionMgrTest passed all 56 tests.
- clang-format v16/check-format on all six changed C++ files, build-support/check-build-hygiene.sh, FE fe-common Maven validate/Checkstyle, and git diff --check passed.
- BE unit-test builds used the temporary apache#70209 fix for an unrelated master compilation failure in vparquet_writer_test.cpp. The prerequisite was restored afterward and is not part of this commit.
- clang-tidy was not completed. A cloud end-to-end regression and a standalone production BE build were not run.

### Release note

Cloud committed-rowset notifications are enabled by default through enable_notify_be_after_load_txn_commit on FE and enable_cloud_make_rs_visible_on_be on BE. Completed notifications release their task signatures, and consumed empty-rowset entries are removed promptly.

### Check List (For Author)

- Test: Unit Test; 36 BE ASAN tests and 56 FE tests passed with the prerequisite described above.
- Behavior changed: Yes (see Details).
- Does this need documentation: Yes; document the two default-value changes in the configuration reference and release notes.
- Pre-submission status: Partially verified: targeted tests passed; cloud end-to-end regression, standalone production BE build and clang-tidy are not completed. The master BE UT build requires apache#70209.

### AI Disclosure

- Tool: Codex (GPT-6).
- Generated: Code, tests and this description.
- Human review: Pending.
### Background

Cloud MoW load initialization can synchronize rowsets from Meta Service before capturing its local rowset snapshot. The existing mutable cloud_mow_sync_rowsets_when_load_txn_begin setting controls this synchronization.

### What problem does this PR solve?

Issue Number: None
Related PR: apache#59754 (introduced the setting), apache#70218 (enables notifications and fixes cleanup), apache#70209 (temporary BE UT build prerequisite)
PR Type: Enhancement

Problem Summary: Make rowset synchronization at the start of a cloud MoW load opt-in by changing cloud_mow_sync_rowsets_when_load_txn_begin from true to false. Adjust the related CloudGroupRowsetBuilderWriterTest fixture to restore the incoming configuration value instead of hard-coding true during teardown.

### Details

- **Before and after:** Cloud MoW loads no longer force sync_rowsets before constructing their initial local rowset snapshot by default.
- **Configs and defaults:** cloud_mow_sync_rowsets_when_load_txn_begin changes from true to false and remains dynamically mutable.
- **Impact on existing users:** An explicit true setting retains synchronization at load initialization. Notification delivery and subsequent metadata synchronization remain relevant to local rowset freshness.
- **Behavior change:** Yes; skip the initial synchronization unless the setting is enabled. The test fixture preserves whichever configuration value it received.

### Testing

- ./run-be-ut.sh --run --filter='CloudGroupRowsetBuilderWriterTest.*:*DeltaWriterCancelIntegrationTest.*' -j100 passed all 12 ASAN tests after this change.
- The unit-test build temporarily applied apache#70209 to fix the unrelated vparquet_writer_test.cpp compilation failure. That prerequisite was removed afterward and is not included in this commit.
- clang-format v16/check-format on both changed files and git diff --check passed.
- The earlier 36 BE and 56 FE passing tests belong to commit 1b4d168, before this default update; they were not rerun for this commit.
- clang-tidy was omitted. The stale-cache partial-update scenario below was not exercised in a running cloud cluster.

### Notes for Reviewers

The stale-rowset partial-update issue identified during source review remains unresolved: if a BE has not received or synchronized a prior committed rowset, constructing the MoW context from its local snapshot can classify an existing key as new and fail a partial update under the ERROR policy or when an omitted column is non-nullable and has no default. This commit changes the default; it does not repair that path.

### Release note

The BE setting cloud_mow_sync_rowsets_when_load_txn_begin now defaults to false. Cloud MoW loads skip forced rowset synchronization at initialization unless this setting is explicitly enabled.

### Check List (For Author)

- Test: Unit Test; 12 BE ASAN tests passed with the prerequisite described above.
- Behavior changed: Yes (see Details).
- Does this need documentation: Yes; include this third default-value change in the configuration reference and release notes alongside the two notification settings.
- Pre-submission status: Partially verified: focused tests passed; the known stale-cache partial-update risk remains unresolved and cloud end-to-end validation is pending.

### AI Disclosure

- Tool: Codex (GPT-6).
- Generated: Code, test fixture adjustment and this description.
- Human review: Pending.
@bobhan1

bobhan1 commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor Author

Updated after rechecking the existing snapshot semantics. The earlier classification of MF-002 as a new P1 is withdrawn; the latest local review is here, for final head 4ba6d0db1480dd018a2e9353cffbb6137fe1c65d.

  • MF-001: False positive. The notification enters AgentBatchTask, without insertion into AgentTaskQueue, so clearing its BE signature does not cause endless FE resends.
  • MF-002: Documented behavior tradeoff of the existing mode. The old initialization sync, snapshot capture and later flush were not atomic with other transactions' commits; keys absent from the captured snapshot already could fail new-key validation. Skipping the sync enlarges the snapshot-lag window, including while an earlier committed rowset is still absent locally. This is the intended default change, and master already ran Cloud p0/p1 with this setting. The early-failure possibility remains; no corrective code for it is claimed. APPEND final-value corruption is not established because publication realigns historical columns. This finding is no longer a blocking P1 under the clarified load semantics.
  • MF-003: Retained as P2 for a new-FE/old-BE rollout window. Enable FE notifications only after all receiving BEs include the signature-cleanup fix.
  • MF-004: False positive. BE UT uses -fno-access-control, and the new callback tests compiled and passed.

Implementation head 17d42f6ef18 passed 46 BE ASAN and 56 FE Cloud transaction tests. The final commit removes six redundant overrides from four Cloud p0/p1 FE/BE config files; their values equal the new defaults, all remaining settings are unchanged, and diff checks passed. Runtime tests were not rerun for this configuration-only cleanup. No cloud end-to-end or mixed-version reproduction was performed, and clang-tidy remained excluded.

### What problem does this PR solve?

Issue Number: None

Related PR: apache#59754

Problem Summary: Cloud p0 and p1 still explicitly configure committed-rowset notifications and initial MoW rowset synchronization to the values now provided by the defaults. Remove these six overrides from the four FE/BE pipeline configuration files so the pipelines exercise the defaults while retaining the same effective settings.

### Release note

None

### Check List (For Author)

- Test: Manual check of all removed values against the FE/BE defaults, comparison of the remaining parsed settings, and git diff --check passed. No runtime rerun for this configuration cleanup because the effective values are unchanged.
- Behavior changed: No (the pipeline settings resolve to the same values through defaults)
- Does this need documentation: No
@bobhan1

bobhan1 commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Local pipeline review — PASS

schema: doris-repo-review/v1
status: PASS
pr: apache/doris#70218
commit: 4ba6d0db1480dd018a2e9353cffbb6137fe1c65d
base: 87b462e4f58104140f098f5f4a8b5d58bec70149
reviewed_at: 2026-11-06T03:55:20+00:00
reviewer: bobhan1
model: gpt-6-astra
effort: max
findings: {blocker: 0, major: 0, minor: 1, nit: 0}
rounds: 1
converged: true

One consolidated review round on the final head, covering all 12 changed files, the FE-to-BE notification lifecycle, cache expiry, configuration defaults and pipeline inheritance. The review incorporates the earlier code-path analysis and the clarified intended snapshot semantics.

Finding Final disposition
MF-001 False positive. The sender uses AgentBatchTask, whose addTask does not insert into AgentTaskQueue. The notification is therefore absent from FE's resend queue.
MF-002 Withdrawn as a new P1; retained as a documented behavior tradeoff. The old sync then snapshot path was not atomic with concurrent commits or the later flush. A key missing from the captured snapshot already could fail new-key validation. Skipping the initialization sync enlarges that snapshot-lag window, including while an earlier committed rowset has not reached local state. The intended change accepts this existing mode as the default; Cloud p0 and Cloud p1 already configured it. The early-failure possibility remains; no corrective code for it is claimed. APPEND's final-value corruption claim is not established because publish alignment rereads historical columns.
MF-003 P2 / Minor, retained. A new FE can send notifications to an old BE that supports this task but lacks its cleanup guard, exposing that old process's signature leak. Keep FE notifications disabled until all receiving BEs contain the cleanup fix.
MF-004 False positive. The BE test target uses -fno-access-control; the actual test compile command includes it, and both new callback cases compiled and passed.

Review checkpoints:

  • Goal and scope: the callback guard releases signatures on enabled, disabled and cache-miss paths; one cache now gives empty and non-empty entries the same removal and expiry lifecycle. The final commit removes only six redundant overrides from four Cloud p0/p1 FE/BE config files, so those pipelines exercise product defaults.
  • Concurrency and lifetime: the existing shared mutex protects the cache and expiry map; refreshed entries retain their authoritative expiry checks. Callback cleanup runs on scope exit after tablet processing. No new nested locks, background threads, static-initialization dependencies or ownership cycles are introduced.
  • Configuration and compatibility: all three settings retain their existing dynamic mutability. No Thrift/Protobuf fields, storage formats or persistence protocols change. The old-BE rollout limitation is the retained P2 above.
  • Transaction and data behavior: notifications remain best effort after the MS commit and do not enter the FE resend queue. Flush snapshot/new-key validation and publish alignment retain their existing algorithms; the initial-sync default changes the freshness/cost tradeoff explicitly described above.
  • Parallel paths and observability: empty and non-empty committed-rowset entries share cleanup; the separate MoW delete-bitmap cache is unchanged. Insertion logging moves outside the cache lock and to DEBUG. Existing transaction/tablet identifiers remain available.
  • Validation and limits: implementation head 17d42f6ef184c7f851920830ad3ce4861fe679aa passed 46/46 targeted BE ASAN tests and 56/56 FE Cloud transaction tests, plus build hygiene and diff checks. The final config-only commit passed checks that the six removed overrides equal the new defaults, all remaining parsed settings are unchanged, and git diff --check is clean. Runtime tests were not repeated for that cleanup. No cloud end-to-end or mixed-version runtime reproduction was performed; clang-tidy was excluded as requested.

@bobhan1

bobhan1 commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 28285 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 4ba6d0db1480dd018a2e9353cffbb6137fe1c65d, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17591	5156	5018	5018
q2	2132	303	243	243
q3	10269	1355	809	809
q4	4677	454	335	335
q5	7428	787	526	526
q6	182	187	156	156
q7	733	780	605	605
q8	9262	1267	996	996
q9	5630	4190	4183	4183
q10	6849	1299	1034	1034
q11	454	238	224	224
q12	636	395	271	271
q13	18105	3547	3038	3038
q14	279	266	252	252
q15	q16	731	704	670	670
q17	1169	745	579	579
q18	6920	6163	6135	6135
q19	1123	958	596	596
q20	380	341	222	222
q21	5268	2143	2275	2143
q22	351	295	250	250
Total cold run time: 100169 ms
Total hot run time: 28285 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	5583	5568	5588	5568
q2	658	590	519	519
q3	5176	5407	4962	4962
q4	1978	2145	1535	1535
q5	5237	5162	5591	5162
q6	261	203	151	151
q7	2320	2097	1915	1915
q8	3025	2666	2629	2629
q9	8049	7970	7991	7970
q10	3696	3895	3289	3289
q11	610	432	402	402
q12	686	711	522	522
q13	3482	3874	3304	3304
q14	327	314	270	270
q15	q16	702	737	641	641
q17	8114	6996	6872	6872
q18	12963	12157	12808	12157
q19	913	751	753	751
q20	2133	2149	1914	1914
q21	5552	4540	4658	4540
q22	488	438	406	406
Total cold run time: 71953 ms
Total hot run time: 65479 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 153989 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 4ba6d0db1480dd018a2e9353cffbb6137fe1c65d, data reload: false

query5	4301	528	404	404
query6	428	202	184	184
query7	4794	437	233	233
query8	323	173	155	155
query9	9315	3266	3232	3232
query10	481	314	257	257
query11	5947	3882	3585	3585
query12	148	95	87	87
query13	1248	433	328	328
query14	6176	3998	3707	3707
query14_1	3434	3500	3460	3460
query15	213	200	187	187
query16	948	437	430	430
query17	852	666	554	554
query18	2434	430	309	309
query19	201	172	141	141
query20	92	89	83	83
query21	209	134	113	113
query22	12999	12976	12875	12875
query23	13504	12766	12544	12544
query23_1	12586	12442	12543	12442
query24	7335	850	464	464
query24_1	472	475	481	475
query25	515	390	350	350
query26	1245	250	142	142
query27	2781	430	269	269
query28	4630	2041	1995	1995
query29	1569	584	460	460
query30	308	215	177	177
query31	1009	929	822	822
query32	140	92	90	90
query33	514	317	237	237
query34	889	854	501	501
query35	737	767	657	657
query36	810	794	792	792
query37	123	100	86	86
query38	1769	1717	1703	1703
query39	794	802	744	744
query39_1	738	720	738	720
query40	219	115	99	99
query41	66	63	63	63
query42	90	84	83	83
query43	347	360	312	312
query44	1344	718	735	718
query45	177	172	170	170
query46	812	961	575	575
query47	2887	3028	2566	2566
query48	311	311	220	220
query49	580	400	302	302
query50	691	297	219	219
query51	10659	10371	10612	10371
query52	80	78	77	77
query53	190	214	171	171
query54	234	204	201	201
query55	72	73	65	65
query56	274	228	200	200
query57	1699	1603	1575	1575
query58	287	265	255	255
query59	2131	2157	2005	2005
query60	286	237	215	215
query61	150	146	138	138
query62	386	340	312	312
query63	191	159	164	159
query64	2747	906	760	760
query65	3354	3293	3260	3260
query66	1758	436	304	304
query67	22679	22715	22518	22518
query68	3023	1029	630	630
query69	392	296	253	253
query70	886	824	819	819
query71	313	241	219	219
query72	2800	2591	2236	2236
query73	520	515	291	291
query74	4644	4546	4349	4349
query75	2366	2275	1903	1903
query76	2317	1049	610	610
query77	351	387	301	301
query78	9568	9644	9146	9146
query79	890	850	538	538
query80	499	420	348	348
query81	508	320	266	266
query82	207	138	117	117
query83	205	203	186	186
query84	291	127	107	107
query85	770	415	377	377
query86	270	240	232	232
query87	1976	1981	1828	1828
query88	3758	2801	2777	2777
query89	317	283	250	250
query90	1906	182	175	175
query91	165	152	125	125
query92	102	93	92	92
query93	985	987	577	577
query94	559	320	277	277
query95	577	405	302	302
query96	698	500	247	247
query97	2442	2501	2317	2317
query98	160	162	150	150
query99	754	760	655	655
Total cold run time: 236666 ms
Total hot run time: 153989 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 24.14 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 4ba6d0db1480dd018a2e9353cffbb6137fe1c65d, data reload: false

query1	0.00	0.00	0.00
query2	0.09	0.05	0.05
query3	0.26	0.08	0.09
query4	1.61	0.09	0.11
query5	0.23	0.24	0.24
query6	1.17	0.69	0.68
query7	0.04	0.01	0.01
query8	0.05	0.03	0.04
query9	0.40	0.35	0.36
query10	0.60	0.57	0.57
query11	0.20	0.15	0.15
query12	0.19	0.14	0.14
query13	0.49	0.49	0.49
query14	1.03	1.00	0.99
query15	0.64	0.60	0.60
query16	0.34	0.36	0.36
query17	1.18	1.14	1.14
query18	0.19	0.18	0.18
query19	2.16	2.00	1.97
query20	0.02	0.02	0.02
query21	15.50	0.18	0.13
query22	4.94	0.05	0.05
query23	16.29	0.32	0.13
query24	1.05	0.24	0.22
query25	0.10	0.04	0.04
query26	0.75	0.21	0.16
query27	0.03	0.04	0.03
query28	3.77	0.77	0.36
query29	12.50	4.16	3.23
query30	0.28	0.13	0.10
query31	2.78	0.55	0.32
query32	3.22	0.60	0.50
query33	3.13	3.23	3.14
query34	15.38	4.01	3.42
query35	3.34	3.37	3.36
query36	0.54	0.48	0.43
query37	0.09	0.07	0.07
query38	0.06	0.04	0.04
query39	0.03	0.03	0.03
query40	0.17	0.15	0.14
query41	0.08	0.03	0.03
query42	0.04	0.03	0.03
query43	0.04	0.04	0.04
Total cold run time: 95 s
Total hot run time: 24.14 s

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 100% (0/0) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 77.93% (36160/46399)
Line Coverage 63.35% (408406/644706)
Region Coverage 60.03% (346453/577136)
Branch Coverage 61.30% (161254/263046)

@bobhan1
bobhan1 merged commit 08231e3 into apache:master Nov 10, 2026
44 of 46 checks passed
bobhan1 added a commit to bobhan1/doris that referenced this pull request Nov 12, 2026
…faults (apache#70218)

After a cloud load transaction commits in Meta Service, FE can notify
the participating BEs of its visible versions. BE uses cached committed
rowsets or lightweight empty-rowset entries to update local tablet
metadata, with normal metadata synchronization as the fallback.

Issue Number: None
Related PR: apache#59754 (introduced the notification path), apache#70209 (fixes the
unrelated Parquet unit-test build failure and is now included in master)
PR Type: Bugfix

Problem Summary: Completed notification tasks retain their registered
signatures, including when the BE feature is disabled. Consumed
empty-rowset markers also remain cached because removal only covers the
separate non-empty-rowset map. Release signatures on every callback exit
and represent both rowset kinds in one cache so removal and expiration
share the same lifecycle. Enable the existing FE and BE notification
settings by default, and default
cloud_mow_sync_rowsets_when_load_txn_begin to false so cloud MoW loads
skip forced synchronization when they begin. Remove the six
now-redundant FE/BE overrides from the Cloud p0/p1 pipeline configs so
those pipelines exercise the new defaults with unchanged effective
settings.

- **Frequency:** Every accepted notification leaves its signature
behind; every consumed empty entry remains until expiration.
- **Root cause:** The callback does not pair task registration with
removal, and the split empty/non-empty caches have different removal
behavior.
- **Why existing tests missed it:** They cover insertion and expiration
but do not check removal of an empty entry or resubmission after a
notification callback completes.
- **Fix:** Add a Defer guard before the BE enable check. Store empty
rowsets as null metadata in the existing committed-rowset cache, reuse
insertion and expiration, and avoid a redundant lookup during cleanup.
- **Other paths and branches:** The guard covers enabled notifications,
disabled notifications and cache misses. Non-empty and empty committed
rowsets now share cleanup; the independent MoW delete-bitmap cache is
unchanged. This change targets master.
- **Behavior change:** `enable_notify_be_after_load_txn_commit` and
`enable_cloud_make_rs_visible_on_be` now default to `true`;
`cloud_mow_sync_rowsets_when_load_txn_begin` now defaults to `false`.
All three settings retain their existing dynamic mutability. The cloud
rowset-builder test fixture now restores the incoming setting rather
than hard-coding `true` at teardown.
- **Concurrency and lifecycle:** Cache operations retain the existing
shared mutex and acquire no new nested locks. Signature cleanup runs
after tablet processing has released its locks. Refreshed entries remain
protected from older expiration records.
- **Compatibility and data correctness:** RPC fields and persistent
formats are unchanged. Cloud MoW loads continue to use a local flush
snapshot. Skipping initialization synchronization enlarges the existing
snapshot freshness window; the partial-update validation and
publish-alignment algorithms are unchanged. The rolling-upgrade caveat
is described below.
- **Observability and cost:** Move per-rowset insertion logging outside
the cache lock and lower it to DEBUG as notifications become enabled by
default. Existing notification and expiration logs retain
transaction/tablet identifiers.

- The implementation commits were rebased onto master
`87b462e4f58104140f098f5f4a8b5d58bec70149`. The BE/FE runs below tested
implementation head `17d42f6ef184c7f851920830ad3ce4861fe679aa`. Final
head `4ba6d0db1480dd018a2e9353cffbb6137fe1c65d` adds only the
pipeline-configuration cleanup.
- On the rebased head, `./run-be-ut.sh --run
--filter='CloudCommittedRSMgrTest.*:EnabledAndDisabled/CloudCommittedRowsetTaskTest.*:TaskWorkerPoolTest.*:CloudTabletApplyVisiblePendingTest.*:CloudGroupRowsetBuilderWriterTest.*:*DeltaWriterCancelIntegrationTest.*'
-j100` passed all 46 ASAN tests with the requested defaults.
- On the same rebased head, `./run-fe-ut.sh --run
org.apache.doris.cloud.transaction.CloudGlobalTransactionMgrTest` passed
all 56 tests (0 failures, errors or skips); the Maven reactor completed
successfully.
- The BE build compiled both `task_worker_pool_test.cpp` and
`vparquet_writer_test.cpp` successfully. The latter uses the apache#70209 fix
already merged into master; no temporary source patch was applied.
- Before the original cleanup fix, the empty-entry removal test and both
enabled/disabled signature-release tests failed; the expiration-refresh
test passed.
- `build-support/check-build-hygiene.sh` and `git diff --check` passed
after rebase. The unchanged PR patch previously passed clang-format
v16/check-format and FE fe-common Maven validate/Checkstyle.
- For the final pipeline cleanup, checked all six removed values against
the FE/BE defaults, compared the remaining parsed settings in all four
files, and ran `git diff --check`: all passed. BE/FE tests were not
rerun for this config-only commit because its effective pipeline values
are unchanged.
- clang-tidy was not rerun, as requested. A cloud end-to-end regression
and a standalone production BE build were not run. The snapshot-window
and mixed-version behavior below was assessed from code, without a
running-cluster reproduction.

Local review of `4ba6d0db1480dd018a2e9353cffbb6137fe1c65d` found no
Blocker or Major issue. One non-blocking P2 remains for rolling
upgrades.

| Existing review finding | Current disposition |
| --- | --- |
|
[MF-001](apache#70218 (comment)):
FE notification retries | False positive: this notification is not
inserted into `AgentTaskQueue`. |
|
[MF-002](apache#70218 (comment)):
stale MoW snapshot | Documented behavior tradeoff of the existing mode;
the new-P1 classification is withdrawn under the intended load
semantics. |
|
[MF-003](apache#70218 (comment)):
old-BE signature leak | P2 remains for a new-FE/old-BE rolling-upgrade
window. |
|
[MF-004](apache#70218 (comment)):
protected test access | False positive: BE UT uses
`-fno-access-control`, and the tests compiled and passed. |

**Snapshot semantics:** Even with initialization synchronization
enabled, `sync_rowsets`, local snapshot construction and the later flush
are not atomic with other transactions' commits. A key absent from the
captured snapshot can already enter the new-key validation path and fail
under ERROR or for an omitted non-nullable column without a default.
Disabling the initial synchronization can enlarge the freshness window,
including while an earlier MS-committed rowset is still absent locally.
This PR deliberately makes that existing mode the default; master
already configures Cloud p0/p1 to use it. This is an explicit
behavior/cost tradeoff, not a claim that local snapshots always contain
the latest committed keys. For APPEND updates that reach publication,
delete-bitmap calculation synchronizes missing versions and realigns
conflicting rows from historical columns; flush-time default/null fill
alone does not establish final data loss.

**Rolling upgrades:** Enabling the new FE notification default while an
old BE supports this task but lacks the cleanup guard exposes the old BE
signature leak, including when its local notification setting is false.
That process retains the signatures until it is restarted/upgraded. Keep
FE notifications disabled until all receiving BEs include the cleanup
fix.

Cloud committed-rowset notifications are enabled by default through
enable_notify_be_after_load_txn_commit on FE and
enable_cloud_make_rs_visible_on_be on BE. Completed notifications
release their task signatures, and consumed empty-rowset entries are
removed promptly. The BE setting
cloud_mow_sync_rowsets_when_load_txn_begin now defaults to false, so
cloud MoW loads skip forced rowset synchronization at initialization
unless it is explicitly enabled.

- Behavior changed: Yes (see Details).
- Does this need documentation: Yes; document all three default-value
changes in the configuration reference and release notes.
- Pre-submission status: Partially verified: the implementation passed
the targeted BE/FE tests and the final pipeline cleanup passed static
effective-configuration checks. Cloud end-to-end regression and a
standalone production BE build were not run; clang-tidy was excluded as
requested. The local review has no Blocker/Major finding, with the
rolling-upgrade P2 and snapshot behavior documented above.

- Tool: Codex (GPT-6).
- Generated: Code, tests and this description.
- Human review: [@gavinchou previously approved implementation head
`17d42f6ef18`](apache#70218 (review));
GitHub dismissed that approval after the pipeline cleanup commit. The
final head has been reviewed locally.

(cherry picked from commit 08231e3)
bobhan1 added a commit to bobhan1/doris that referenced this pull request Nov 13, 2026
…faults (apache#70218)

After a cloud load transaction commits in Meta Service, FE can notify
the participating BEs of its visible versions. BE uses cached committed
rowsets or lightweight empty-rowset entries to update local tablet
metadata, with normal metadata synchronization as the fallback.

Issue Number: None
Related PR: apache#59754 (introduced the notification path), apache#70209 (fixes the
unrelated Parquet unit-test build failure and is now included in master)
PR Type: Bugfix

Problem Summary: Completed notification tasks retain their registered
signatures, including when the BE feature is disabled. Consumed
empty-rowset markers also remain cached because removal only covers the
separate non-empty-rowset map. Release signatures on every callback exit
and represent both rowset kinds in one cache so removal and expiration
share the same lifecycle. Enable the existing FE and BE notification
settings by default, and default
cloud_mow_sync_rowsets_when_load_txn_begin to false so cloud MoW loads
skip forced synchronization when they begin. Remove the six
now-redundant FE/BE overrides from the Cloud p0/p1 pipeline configs so
those pipelines exercise the new defaults with unchanged effective
settings.

- **Frequency:** Every accepted notification leaves its signature
behind; every consumed empty entry remains until expiration.
- **Root cause:** The callback does not pair task registration with
removal, and the split empty/non-empty caches have different removal
behavior.
- **Why existing tests missed it:** They cover insertion and expiration
but do not check removal of an empty entry or resubmission after a
notification callback completes.
- **Fix:** Add a Defer guard before the BE enable check. Store empty
rowsets as null metadata in the existing committed-rowset cache, reuse
insertion and expiration, and avoid a redundant lookup during cleanup.
- **Other paths and branches:** The guard covers enabled notifications,
disabled notifications and cache misses. Non-empty and empty committed
rowsets now share cleanup; the independent MoW delete-bitmap cache is
unchanged. This change targets master.
- **Behavior change:** `enable_notify_be_after_load_txn_commit` and
`enable_cloud_make_rs_visible_on_be` now default to `true`;
`cloud_mow_sync_rowsets_when_load_txn_begin` now defaults to `false`.
All three settings retain their existing dynamic mutability. The cloud
rowset-builder test fixture now restores the incoming setting rather
than hard-coding `true` at teardown.
- **Concurrency and lifecycle:** Cache operations retain the existing
shared mutex and acquire no new nested locks. Signature cleanup runs
after tablet processing has released its locks. Refreshed entries remain
protected from older expiration records.
- **Compatibility and data correctness:** RPC fields and persistent
formats are unchanged. Cloud MoW loads continue to use a local flush
snapshot. Skipping initialization synchronization enlarges the existing
snapshot freshness window; the partial-update validation and
publish-alignment algorithms are unchanged. The rolling-upgrade caveat
is described below.
- **Observability and cost:** Move per-rowset insertion logging outside
the cache lock and lower it to DEBUG as notifications become enabled by
default. Existing notification and expiration logs retain
transaction/tablet identifiers.

- The implementation commits were rebased onto master
`87b462e4f58104140f098f5f4a8b5d58bec70149`. The BE/FE runs below tested
implementation head `17d42f6ef184c7f851920830ad3ce4861fe679aa`. Final
head `4ba6d0db1480dd018a2e9353cffbb6137fe1c65d` adds only the
pipeline-configuration cleanup.
- On the rebased head, `./run-be-ut.sh --run
--filter='CloudCommittedRSMgrTest.*:EnabledAndDisabled/CloudCommittedRowsetTaskTest.*:TaskWorkerPoolTest.*:CloudTabletApplyVisiblePendingTest.*:CloudGroupRowsetBuilderWriterTest.*:*DeltaWriterCancelIntegrationTest.*'
-j100` passed all 46 ASAN tests with the requested defaults.
- On the same rebased head, `./run-fe-ut.sh --run
org.apache.doris.cloud.transaction.CloudGlobalTransactionMgrTest` passed
all 56 tests (0 failures, errors or skips); the Maven reactor completed
successfully.
- The BE build compiled both `task_worker_pool_test.cpp` and
`vparquet_writer_test.cpp` successfully. The latter uses the apache#70209 fix
already merged into master; no temporary source patch was applied.
- Before the original cleanup fix, the empty-entry removal test and both
enabled/disabled signature-release tests failed; the expiration-refresh
test passed.
- `build-support/check-build-hygiene.sh` and `git diff --check` passed
after rebase. The unchanged PR patch previously passed clang-format
v16/check-format and FE fe-common Maven validate/Checkstyle.
- For the final pipeline cleanup, checked all six removed values against
the FE/BE defaults, compared the remaining parsed settings in all four
files, and ran `git diff --check`: all passed. BE/FE tests were not
rerun for this config-only commit because its effective pipeline values
are unchanged.
- clang-tidy was not rerun, as requested. A cloud end-to-end regression
and a standalone production BE build were not run. The snapshot-window
and mixed-version behavior below was assessed from code, without a
running-cluster reproduction.

Local review of `4ba6d0db1480dd018a2e9353cffbb6137fe1c65d` found no
Blocker or Major issue. One non-blocking P2 remains for rolling
upgrades.

| Existing review finding | Current disposition |
| --- | --- |
|
[MF-001](apache#70218 (comment)):
FE notification retries | False positive: this notification is not
inserted into `AgentTaskQueue`. |
|
[MF-002](apache#70218 (comment)):
stale MoW snapshot | Documented behavior tradeoff of the existing mode;
the new-P1 classification is withdrawn under the intended load
semantics. |
|
[MF-003](apache#70218 (comment)):
old-BE signature leak | P2 remains for a new-FE/old-BE rolling-upgrade
window. |
|
[MF-004](apache#70218 (comment)):
protected test access | False positive: BE UT uses
`-fno-access-control`, and the tests compiled and passed. |

**Snapshot semantics:** Even with initialization synchronization
enabled, `sync_rowsets`, local snapshot construction and the later flush
are not atomic with other transactions' commits. A key absent from the
captured snapshot can already enter the new-key validation path and fail
under ERROR or for an omitted non-nullable column without a default.
Disabling the initial synchronization can enlarge the freshness window,
including while an earlier MS-committed rowset is still absent locally.
This PR deliberately makes that existing mode the default; master
already configures Cloud p0/p1 to use it. This is an explicit
behavior/cost tradeoff, not a claim that local snapshots always contain
the latest committed keys. For APPEND updates that reach publication,
delete-bitmap calculation synchronizes missing versions and realigns
conflicting rows from historical columns; flush-time default/null fill
alone does not establish final data loss.

**Rolling upgrades:** Enabling the new FE notification default while an
old BE supports this task but lacks the cleanup guard exposes the old BE
signature leak, including when its local notification setting is false.
That process retains the signatures until it is restarted/upgraded. Keep
FE notifications disabled until all receiving BEs include the cleanup
fix.

Cloud committed-rowset notifications are enabled by default through
enable_notify_be_after_load_txn_commit on FE and
enable_cloud_make_rs_visible_on_be on BE. Completed notifications
release their task signatures, and consumed empty-rowset entries are
removed promptly. The BE setting
cloud_mow_sync_rowsets_when_load_txn_begin now defaults to false, so
cloud MoW loads skip forced rowset synchronization at initialization
unless it is explicitly enabled.

- Behavior changed: Yes (see Details).
- Does this need documentation: Yes; document all three default-value
changes in the configuration reference and release notes.
- Pre-submission status: Partially verified: the implementation passed
the targeted BE/FE tests and the final pipeline cleanup passed static
effective-configuration checks. Cloud end-to-end regression and a
standalone production BE build were not run; clang-tidy was excluded as
requested. The local review has no Blocker/Major finding, with the
rolling-upgrade P2 and snapshot behavior documented above.

- Tool: Codex (GPT-6).
- Generated: Code, tests and this description.
- Human review: [@gavinchou previously approved implementation head
`17d42f6ef18`](apache#70218 (review));
GitHub dismissed that approval after the pipeline cleanup commit. The
final head has been reviewed locally.

(cherry picked from commit 08231e3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants