bobhan1 commented on PR #70218: URL: https://github.com/apache/doris/pull/70218#issuecomment-6676267686
Local review of head `17d42f6ef184c7f851920830ad3ce4861fe679aa` against master `87b462e4f58104140f098f5f4a8b5d58bec70149`. The review still has one P1 and one P2. The two other findings are false positives. | Finding | Disposition | Code evidence | | --- | --- | --- | | [MF-001](https://github.com/apache/doris/pull/70218#discussion_r4476514236) | False positive | [The sender](https://github.com/apache/doris/blob/17d42f6ef184c7f851920830ad3ce4861fe679aa/fe/fe-core/src/main/java/org/apache/doris/cloud/transaction/CloudGlobalTransactionMgr.java#L3092) adds the notification only to `AgentBatchTask`. [Its addTask method](https://github.com/apache/doris/blob/17d42f6ef184c7f851920830ad3ce4861fe679aa/fe/fe-core/src/main/java/org/apache/doris/task/AgentBatchTask.java#L98) stores the task in its own map, without registering it in `AgentTaskQueue`. `ReportHandler` resends entries from `AgentTaskQueue`, so removing the BE signature does not cause endless FE retries. | | [MF-002](https://github.com/apache/doris/pull/70218#discussion_r4476514368) | P1 remains valid for early load failure; narrow the APPEND consequence | With the new default, [CloudRowsetBuilder::init](https://github.com/apache/doris/blob/17d42f6ef184c7f851920830ad3ce4861fe679aa/be/src/cloud/cloud_rowset_builder.cpp#L63) can capture stale local rowsets when a previous commit notification is delayed or missed. An existing key can then enter [handle_new_key](https://github.com/apache/doris/blob/17d42f6ef184c7f851920830ad3ce4861fe679aa/be/src/storage/partial_update_info.cpp#L242), failing under ERROR or for an omitted non-nullable column without a default. APPEND's flush-time default/null fill alone does not prove final data loss: [delete-bitmap calculation synchronizes missing versions](https://github.com/apache/doris/blob/17d42f6ef184c7f851920830ad3ce4861fe679aa/be/src/cloud/cloud_engine_calc_delete_bitmap_task.cpp#L175), and [publish alignment reads historical columns](https://git hub.com/apache/doris/blob/17d42f6ef184c7f851920830ad3ce4861fe679aa/be/src/storage/tablet/base_tablet.cpp#L1398). That later work cannot rescue a load that failed during flush. | | [MF-003](https://github.com/apache/doris/pull/70218#discussion_r4476514518) | P2 remains valid during a mixed-version rollout | A new FE can notify an old BE that supports the task but lacks the cleanup guard. [The pre-fix callback](https://github.com/apache/doris/blob/87b462e4f58104140f098f5f4a8b5d58bec70149/be/src/agent/task_worker_pool.cpp#L2595) leaves the registered signature behind, including when its BE feature flag is false. The leak lasts for that old process's lifetime. The guard fixes upgraded BEs; it cannot change already-running old BEs. | | [MF-004](https://github.com/apache/doris/pull/70218#discussion_r4476514655) | False positive | [The BE unit-test target](https://github.com/apache/doris/blob/17d42f6ef184c7f851920830ad3ce4861fe679aa/be/test/CMakeLists.txt#L169) uses `-fno-access-control`. The actual compile command includes that flag, and the new enabled/disabled callback tests compiled and passed. | Validation already completed on this exact head: 46/46 targeted BE ASAN tests and 56/56 `CloudGlobalTransactionMgrTest` tests passed; build hygiene and `git diff --check` passed. The Parquet unit-test compilation issue is fixed by #70209 in master, with no temporary patch. The stale-snapshot and mixed-version scenarios above are source-based findings, without a running-cluster reproduction. clang-tidy was excluded as requested. The [local-review validator](https://github.com/apache/doris/blob/87b462e4f58104140f098f5f4a8b5d58bec70149/.github/scripts/validate_review_pass_comment.py#L159) requires both Blocker and Major findings to be zero. A valid local PASS can satisfy the aggregate check, but the remaining P1 prevents an honest PASS report for this head. Suggested resolution: preserve initial synchronization for partial updates (or add a freshness fallback); during rolling upgrades, enable FE notifications only after all receiving BEs contain the cleanup fix. The requested configuration defaults are unchanged by this review. -- 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]
