Repository navigation
branch-4.2: [fix](cloud) Fix rowset notification cleanup and update cloud load defaults #70218 - #70455
Open
bobhan1 wants to merge 1 commit into
Conversation
bobhan1
marked this pull request as ready for review
November 11, 2026 03:14
Contributor
|
run beut |
bobhan1
force-pushed
the
backport/4.2/70218-cloud-rowset-notification-cleanup
branch
from
November 12, 2026 06:05
5088100 to
3687454
Compare
Contributor
Author
|
run buildall |
…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
force-pushed
the
backport/4.2/70218-cloud-rowset-notification-cleanup
branch
from
November 13, 2026 03:34
3687454 to
cb5da41
Compare
Contributor
Author
|
run buildall |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: #70218
Backport #70218 (
08231e3a6359082264c88e6fe2c8050e911e5236) tobranch-4.2.Completed committed-rowset notifications retain task signatures, and consumed empty-rowset markers remain cached. Release signatures on every callback exit and give empty and non-empty rowsets the same removal and expiration lifecycle. Enable the existing FE/BE notification settings by default, disable forced initial MoW rowset synchronization by default, and remove the six redundant Cloud p0/p1 configuration overrides.
Branch-specific adaptations
CloudGroupRowsetBuilderWriterTest: that test and its corresponding feature are absent frombranch-4.2. The notification and cache regression tests are identical to the upstream versions.Testing
./run-fe-ut.sh --run org.apache.doris.cloud.transaction.CloudGlobalTransactionMgrTest: 33 tests passed, 0 failures/errors/skips; MavenBUILD SUCCESS../run-be-ut.sh --run --filter='CloudCommittedRSMgrTest.*:EnabledAndDisabled/CloudCommittedRowsetTaskTest.*:TaskWorkerPoolTest.*:CloudTabletApplyVisiblePendingTest.*:*DeltaWriterCancelTest.*' -j100: 44 ASAN tests passed, including both enabled/disabled signature-release cases, empty-rowset removal, expiration refresh, pending-rowset visibility, and local/cloud cancellation.git diff --check upstream/branch-4.2...HEADpassed.Release note
Cloud committed-rowset notifications now default to enabled: FE
enable_notify_be_after_load_txn_commit=trueand BEenable_cloud_make_rs_visible_on_be=true. Completed notifications release their task signatures, and consumed empty-rowset entries are removed promptly. BEcloud_mow_sync_rowsets_when_load_txn_beginnow defaults tofalse.The original PR's behavior and upgrade considerations apply: skipping initial synchronization can enlarge the existing local-snapshot freshness window. During rolling upgrades, keep FE notifications disabled until all receiving BEs contain the signature-cleanup fix.
Check List (For Author)
AI Disclosure