Skip to content

[fix](test) Build vparquet_writer_test against the location-based Iceberg WriteInfo - #70209

Merged
morningman merged 1 commit into
apache:masterfrom
morningman:b2m-fix-vparquet-writer-test-write-info
Nov 4, 2026
Merged

morningman merged 1 commit into
apache:masterfrom
morningman:b2m-fix-vparquet-writer-test-write-info

Conversation

@morningman

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: None

Related PR: #69392, #69971

Problem Summary:

In short. doris_be_test does not compile on master since #69971 was merged, so the BE UT of every PR fails with BE BUILD FAIL. This fixes the one test case that breaks it.

The problem. #69392 replaced the write_path, original_write_path and target_path fields of IPartitionWriterBase::WriteInfo with one IcebergOutputLocation location, and updated the callers it knew about. #69971, merged after it, added VParquetWriterTest.HiveAndIcebergParquetPageIndexOptions, which still sets the old fields:

be/test/format/transformer/vparquet_writer_test.cpp:380:41: error: no matching constructor for initialization of 'VIcebergPartitionWriter'

Each PR built alone, so both were green. TeamCity's BE UT has failed this way for PRs merged with master 0eb5131db62 (for example build 1092024 of #69605).

How this PR fixes it. The case builds its WriteInfo as the other Iceberg case in the same file does, .location = IcebergOutputLocation(".", "."). Nothing else changes.

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason

    doris_be_test builds again with this change, and VParquetWriterTest (16 tests) passes, on a local ASAN build (macOS).

  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

…berg WriteInfo

### What problem does this PR solve?

Issue Number: None

Related PR: apache#69392, apache#69971

Problem Summary: apache#69392 replaced the `write_path`, `original_write_path`
and `target_path` fields of `IPartitionWriterBase::WriteInfo` with one
`IcebergOutputLocation location`. apache#69971, merged after it, added
`VParquetWriterTest.HiveAndIcebergParquetPageIndexOptions`, which still
sets the old fields. Since then `doris_be_test` does not compile on
master (`vparquet_writer_test.cpp:380: no matching constructor for
initialization of 'VIcebergPartitionWriter'`), so the BE UT of every
PR fails with BE BUILD FAIL. The case now builds its `WriteInfo` as the
other Iceberg case in the same file does,
`.location = IcebergOutputLocation(".", ".")`.

### Release note

None

### Check List (For Author)

- Test: Unit Test
    - `doris_be_test` builds again, and `VParquetWriterTest` passes
      (local ASAN build, macOS).
- Behavior changed: No
- Does this need documentation: No

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@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?

@morningman

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

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

------ Round 1 ----------------------------------
============================================
q1	17616	5152	5034	5034
q2	2143	310	260	260
q3	10247	1380	839	839
q4	4680	458	337	337
q5	7449	789	539	539
q6	185	181	153	153
q7	744	785	610	610
q8	9259	1335	1085	1085
q9	5670	4185	4165	4165
q10	6826	1294	1042	1042
q11	432	248	225	225
q12	643	396	276	276
q13	18121	3783	3048	3048
q14	276	277	257	257
q15	q16	737	714	673	673
q17	1203	754	600	600
q18	7009	6151	6141	6141
q19	1270	966	591	591
q20	401	335	230	230
q21	6120	2401	2409	2401
q22	388	324	290	290
Total cold run time: 101419 ms
Total hot run time: 28796 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	5972	5968	6145	5968
q2	745	613	597	597
q3	5660	5874	5255	5255
q4	2038	2128	1525	1525
q5	5779	5926	5699	5699
q6	242	189	138	138
q7	2240	2100	1838	1838
q8	2962	2689	2645	2645
q9	8184	7808	7565	7565
q10	3635	3545	3113	3113
q11	584	400	374	374
q12	675	674	504	504
q13	3244	3609	3044	3044
q14	298	285	262	262
q15	q16	652	675	618	618
q17	7750	7021	6992	6992
q18	12950	12226	12876	12226
q19	890	763	769	763
q20	2113	2156	1895	1895
q21	5571	4585	4655	4585
q22	481	455	406	406
Total cold run time: 72665 ms
Total hot run time: 66012 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 150953 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 66ae368f78d236124f7c1786303bd7d5669166fd, data reload: false

query5	4299	601	418	418
query6	424	229	195	195
query7	4804	453	232	232
query8	326	171	156	156
query9	8774	3198	3205	3198
query10	483	313	251	251
query11	6074	3869	3585	3585
query12	147	90	87	87
query13	1258	459	330	330
query14	6135	3926	3649	3649
query14_1	3391	3466	3409	3409
query15	213	193	180	180
query16	955	436	409	409
query17	841	664	509	509
query18	2422	420	301	301
query19	187	169	155	155
query20	83	81	79	79
query21	212	139	113	113
query22	13136	13116	12833	12833
query23	13407	12843	12390	12390
query23_1	12605	12465	12525	12465
query24	7376	805	485	485
query24_1	480	483	468	468
query25	518	386	343	343
query26	1260	256	132	132
query27	2779	443	284	284
query28	4652	2028	2011	2011
query29	1539	580	431	431
query30	310	229	183	183
query31	1002	920	810	810
query32	155	94	91	91
query33	516	298	234	234
query34	1002	857	493	493
query35	737	754	654	654
query36	827	820	736	736
query37	123	98	85	85
query38	1795	1737	1671	1671
query39	792	789	742	742
query39_1	731	724	747	724
query40	215	116	96	96
query41	68	65	68	65
query42	94	88	83	83
query43	346	361	311	311
query44	1340	712	723	712
query45	188	181	184	181
query46	846	975	572	572
query47	3058	3029	2526	2526
query48	327	324	227	227
query49	619	415	305	305
query50	686	290	220	220
query51	10475	10673	10461	10461
query52	79	85	72	72
query53	196	212	155	155
query54	247	209	222	209
query55	76	70	65	65
query56	254	243	242	242
query57	1667	1661	1591	1591
query58	297	273	269	269
query59	2163	2208	1989	1989
query60	287	254	242	242
query61	171	169	172	169
query62	407	334	289	289
query63	191	161	164	161
query64	3008	1066	856	856
query65	3362	3288	3275	3275
query66	1801	417	298	298
query67	20145	20158	19863	19863
query68	3177	1011	608	608
query69	405	287	247	247
query70	852	784	781	781
query71	294	230	208	208
query72	2783	2568	2257	2257
query73	527	523	299	299
query74	4669	4509	4354	4354
query75	2347	2283	1911	1911
query76	2383	1051	577	577
query77	343	381	275	275
query78	9735	9732	9141	9141
query79	998	846	533	533
query80	939	421	337	337
query81	591	327	266	266
query82	753	130	111	111
query83	312	197	191	191
query84	327	131	100	100
query85	939	437	370	370
query86	367	240	230	230
query87	2002	1927	1812	1812
query88	3734	2751	2743	2743
query89	336	283	242	242
query90	1792	170	170	170
query91	166	147	127	127
query92	102	83	90	83
query93	995	998	583	583
query94	543	308	276	276
query95	588	330	364	330
query96	683	487	234	234
query97	2493	2436	2377	2377
query98	164	160	150	150
query99	768	771	647	647
Total cold run time: 236302 ms
Total hot run time: 150953 ms

@hello-stephen

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

query1	0.01	0.00	0.01
query2	0.09	0.05	0.05
query3	0.26	0.09	0.10
query4	1.61	0.11	0.11
query5	0.22	0.24	0.24
query6	1.17	0.68	0.67
query7	0.03	0.01	0.01
query8	0.05	0.04	0.04
query9	0.40	0.36	0.34
query10	0.58	0.59	0.61
query11	0.20	0.14	0.14
query12	0.18	0.15	0.15
query13	0.48	0.50	0.49
query14	1.05	1.00	1.00
query15	0.64	0.62	0.61
query16	0.31	0.34	0.35
query17	1.18	1.13	1.16
query18	0.20	0.18	0.19
query19	2.13	1.98	1.98
query20	0.02	0.01	0.01
query21	15.50	0.19	0.14
query22	4.89	0.05	0.06
query23	16.28	0.32	0.12
query24	3.06	0.67	0.50
query25	0.14	0.05	0.06
query26	1.50	0.24	0.16
query27	0.05	0.04	0.05
query28	3.06	0.72	0.36
query29	12.47	4.14	3.25
query30	0.28	0.13	0.09
query31	2.77	0.57	0.33
query32	3.22	0.61	0.50
query33	3.15	3.19	3.27
query34	15.47	3.99	3.41
query35	3.36	3.34	3.35
query36	0.55	0.48	0.45
query37	0.09	0.07	0.06
query38	0.04	0.04	0.03
query39	0.03	0.03	0.03
query40	0.17	0.15	0.14
query41	0.09	0.03	0.03
query42	0.04	0.03	0.03
query43	0.04	0.04	0.03
Total cold run time: 97.06 s
Total hot run time: 24.5 s

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.
@morningman

Copy link
Copy Markdown
Contributor Author

skip buildall

@morningman
morningman merged commit 8e499c0 into apache:master Nov 4, 2026
37 checks passed
@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 67.40% (32167/47727)
Line Coverage 52.27% (338746/648100)
Region Coverage 47.79% (275433/576344)
Branch Coverage 49.60% (129808/261690)

bobhan1 added a commit to bobhan1/doris that referenced this pull request Nov 6, 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.
bobhan1 added a commit to bobhan1/doris that referenced this pull request Nov 6, 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 added a commit that referenced this pull request Nov 10, 2026
…faults (#70218)

### 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 #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](#70218 (comment)):
FE notification retries | False positive: this notification is not
inserted into `AgentTaskQueue`. |
|
[MF-002](#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](#70218 (comment)):
old-BE signature leak | P2 remains for a new-FE/old-BE rolling-upgrade
window. |
|
[MF-004](#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.

### 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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants