luwei16 commented on PR #68793:
URL: https://github.com/apache/doris/pull/68793#issuecomment-6075767735
## Local review of PR #68793
Reviewed the complete six-file three-dot diff from base
`038e2274ef678c44f85214e31da6d3e1d48a794f` to head
`6fe5c44c417c71c5d82ae4bc22bd6ae90032e785`. Two rounds of main-agent review and
three independent full-review subagents per round completed; the final round
returned `NO_NEW_VALUABLE_FINDINGS` from all three subagents. Final sweep found
no unresolved candidates or Blocker/Major/Minor/Nit findings.
The final round used runtime-recorded `gpt-6.1-sol` at `xhigh`. The first
round started at `high` and was interrupted; its recovery used `xhigh`. A
complete additional round was performed at `xhigh` rather than relabeling the
earlier round. This is a code-review result, not a claim that every CI check
has passed.
### Critical checkpoints
| Checkpoint | Conclusion |
|---|---|
| Goal and correctness | The three one-to-one metadata rewrites preserve
source commit TSO. Index tests exercise actual placeholder-zero segments,
fresh-reader projection and pruning; snapshot tests check converted metadata. |
| Minimality and reuse | Production changes are three guarded copies, nine
lines total. Index changes reuse the existing manual-build metadata helper;
generic writer/reader behavior is unchanged. |
| Concurrency and locking | Only private output metadata is mutated.
Published input rowsets, existing task locks and rowset-update/meta lock order
remain unchanged; no new threads, locks or heavyweight operations are
introduced. |
| Lifecycle and static initialization | Shared rowset ownership, pending
guards and unused-rowset tracking remain intact. The local snapshot's transient
rowset is not published; load only changes rowset state. No new static
initialization dependency or reference cycle exists. |
| Configuration | No new configuration or dynamic-update behavior. |
| Interface and storage compatibility | No protobuf schema, RPC, symbol or
storage-format change. Absent fields remain absent; explicit unassigned values
and complete ranges retain their meaning. The reader's -1 compatibility
behavior is unchanged. |
| Parallel paths | BUILD/DROP and index formats share the fixed metadata
path. Local visible/stale paths share rename; remote rowsets already copy
complete PBs. Cloud compaction already sets output TSO ranges, and Cloud PB
converters preserve the copied field. |
| Conditions and errors | Presence guards represent valid legacy/prepublish
states, not speculative defensive error handling. New test Status/Result
accesses are checked before use. |
| Test coverage and negative boundaries | Tests distinguish absent, explicit
[-1,-1], assigned single values and snapshot ranges. DROP independently seeds
its source TSO; fresh Segment avoids stale-reader masking. Cloud uses nonzero
physical segment ID 7. |
| Test results | No handwritten regression outputs. Existing final XML/log
evidence was checked: baseline 6 failures of 8, fixed 8/8 passing, related
suites 54/54 passing. No tests were rerun during this read-only review. |
| Observability | No new execution stage, error category or distributed
operation; existing task/snapshot logs suffice. Additional metrics are not
required for a fixed-size metadata copy. |
| Transactions and persistence | Index output enters the existing
replacement and production save path. Local TSO is copied before header
serialization; Cloud conversion and existing restore PB serialization retain
it. No FE EditLog or transaction protocol change. |
| Atomicity, crashes and MoW | Segment content, versions, bitmap remapping,
pending guards, replacement, file cleanup and persistence ordering are
unchanged; no new partial-commit window is introduced. |
| FE-BE propagation | No new transmitted variable. The existing optional PB
field and converters are reused. |
| Performance and memory | Constant-size per-rowset copies, not per-row
work; no new scan, RPC, retry or significant allocation. Test
iterator/column/predicate ownership is sound. |
| Other module obligations | No hub header, PCH, extern-template,
unity-skip, hygiene-gate or libc/link dependency change. Physical IDs are not
confused with segment positions. No other substantiated issue remains. |
Relevant flow:
```text
visible rowset -> index spec -> manual_build -> same-version replace ->
production save_meta
local visible/stale -> rename/build transient rowset -> output PB + TSO ->
persisted header
Cloud source -> fresh output PB + TSO -> existing PB conversion -> restore
metadata persistence
```
### Verification boundaries
This review was read-only: no build, test execution, source modification or
GitHub publication. Existing test evidence is from the implementation phase.
Index UT does not execute production save_meta under BE_TEST, so it is not a
restart-persistence test. Local snapshot UT uses empty rowsets to check
visible/stale header rewriting. Cloud UT verifies metadata conversion, not
actual S3/MS RPC or Cloud SQL RESTORE; active Row Binlog source/target restore
is rejected by FE. The patch prevents future metadata omissions, not recovery
of already-lost historical timestamps.
<!-- doris-repo-review:v1:begin -->
```yaml
schema: doris-repo-review/v1
status: PASS
pr: apache/doris#68793
commit: 6fe5c44c417c71c5d82ae4bc22bd6ae90032e785
base: 038e2274ef678c44f85214e31da6d3e1d48a794f
reviewed_at: 2026-10-09T06:32:34+00:00
reviewer: luwei16
model: gpt-6.1-sol
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 2
converged: true
```
<!-- doris-repo-review:v1:end -->
--
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]
