wenzhenghu commented on PR #67143: URL: https://github.com/apache/doris/pull/67143#issuecomment-5420164818
Reviewed the current head `aecf7779e87354bc7f8f1f704b614c92827854f7` against `master@272596237e9cfe2504289bfe3ce539d8027cc528`. No blocking issue was found. LGTM from the static review perspective. Key conclusions: - The statuses returned by `BaseTablet::commit_phase_update_delete_bitmap()` and `CalcDeleteBitmapToken::wait()` are both propagated correctly. - Calling `wait()` even after a synchronous submission failure is necessary: tasks already submitted to the token must finish before the rowset, segments, and downloaded files can be released or removed. - A delete bitmap failure returns before `commit_txn()`. The existing deferred handler then aborts the local tablet transaction, so an incomplete MOW delete bitmap cannot be attached to a committed rowset through this path. - Capturing `download_success_files` by reference is lifetime-safe here and allows the failure handler to remove the files actually downloaded during the ingest. A secondary `batch_delete()` failure is logged without replacing the original ingest error. - The ordinary `BaseRowsetBuilder` path already propagates both submission and wait failures, so limiting this change to the Shared-Nothing CCR `IngestBinlog` path is appropriately scoped. - No new lock ordering, storage-format, Thrift/protocol, configuration, or rolling-upgrade compatibility change is introduced. The token is drained before failure cleanup, and no new concurrency or object-lifetime issue was found. - The regression case exercises the asynchronous delete bitmap failure, verifies that ingest reports failure and the target version is not advanced, and verifies that retrying the same binlog succeeds without exposing duplicate unique keys. Validation performed in this review: - Confirmed the exact PR head/base and the two-file PR diff (`+61/-6`). - Traced the Shared-Nothing ingest path, `CalcDeleteBitmapToken`, `BaseTablet::commit_phase_update_delete_bitmap()`, `TxnManager`, and the ordinary rowset-builder path. - `git diff --check` passed for the PR diff. - This was a static review; I did not independently run a BE build, BE unit tests, or the two-cluster CCR regression suite. -- 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]
