bobhan1 commented on PR #70218:
URL: https://github.com/apache/doris/pull/70218#issuecomment-6676454902
<!-- doris-repo-review:v1:begin -->
### Local pipeline review — PASS
```yaml
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
```
<!-- doris-repo-review:v1:end -->
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](https://github.com/apache/doris/pull/70218#discussion_r4476514236) |
False positive. [The
sender](https://github.com/apache/doris/blob/4ba6d0db1480dd018a2e9353cffbb6137fe1c65d/fe/fe-core/src/main/java/org/apache/doris/cloud/transaction/CloudGlobalTransactionMgr.java#L3092)
uses `AgentBatchTask`, whose
[addTask](https://github.com/apache/doris/blob/4ba6d0db1480dd018a2e9353cffbb6137fe1c65d/fe/fe-core/src/main/java/org/apache/doris/task/AgentBatchTask.java#L98)
does not insert into `AgentTaskQueue`. The notification is therefore absent
from FE's resend queue. |
|
[MF-002](https://github.com/apache/doris/pull/70218#discussion_r4476514368) |
Withdrawn as a new P1; retained as a documented behavior tradeoff. The old
[sync then snapshot
path](https://github.com/apache/doris/blob/87b462e4f58104140f098f5f4a8b5d58bec70149/be/src/cloud/cloud_rowset_builder.cpp#L63)
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](https://github.com/apache/doris/blob/87b462e4f58104140f098f5f4a8b5d58bec70149/regression-test/pipeline/cloud_p0/conf/be_custom.conf#L70)
and [Cloud
p1](https://github.com/apache/doris/blob/87b462e4f58104140f098f5f4a8b5d58bec70149/regression-test/pipeline/cloud_p1/conf/be_custom.conf#L50)
already configured it. The early-failure possibili
ty remains; no corrective code for it is claimed. APPEND's final-value
corruption claim is not established because [publish alignment rereads
historical
columns](https://github.com/apache/doris/blob/4ba6d0db1480dd018a2e9353cffbb6137fe1c65d/be/src/storage/tablet/base_tablet.cpp#L1398).
|
|
[MF-003](https://github.com/apache/doris/pull/70218#discussion_r4476514518) |
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](https://github.com/apache/doris/pull/70218#discussion_r4476514655) |
False positive. The [BE test
target](https://github.com/apache/doris/blob/4ba6d0db1480dd018a2e9353cffbb6137fe1c65d/be/test/CMakeLists.txt#L169)
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
