wyxxxcat commented on PR #69264: URL: https://github.com/apache/doris/pull/69264#issuecomment-6621346461
Reviewed PR #69264 at head `231d414a494e06aef5b3dfa43c02cc99b7d9eaad`, against base `73d9733c78a48c82f04aa5f70fe118bebd11b6f4` (five changed files, three commits). **Standards:** No additional substantive convention or maintainability findings. **Review context:** The author clarified that no new ingestion occurs while the check runs. The first finding below applies if background compaction is still running; it does not require concurrent ingestion. If compaction and other rowset-producing jobs are also quiesced, that finding does not apply to this operational scenario. **Spec / correctness:** Two previously reported P2 concerns remain in the current head, with the first narrowed to the compaction scenario described below. These are carried forward here, rather than posted as duplicate inline threads. 1. **P2 — Revalidate cached rowsets when the bitmap snapshot changes.** [checker.cpp:1504–1547](https://github.com/apache/doris/blob/231d414a494e06aef5b3dfa43c02cc99b7d9eaad/cloud/src/recycler/checker.cpp#L1504-L1547). The requirement is to stop at a bitmap “without a corresponding formal meta rowset.” Each bitmap page creates a new transaction at line 1462, but the rowset cache is rebuilt only when the tablet changes. With ingestion stopped, an ongoing cumulative/base compaction can still generate and publish a new output rowset between successful checker pages. For example, version 20 already overwrote an input row before checking began, while compaction processes older versions [2,10]. The output retains that row at its own version and receives a converted version-20 delete bitmap. [CloudTablet::calc_delete_bitmap_for_compaction](https://github.com/apache/doris/blob/231d414a494e06aef5b3dfa43c02cc99b7d9eaad/be/src/cloud/cloud_tablet.cpp#L1394-L1401) converts historical bitmaps to the output ID and persists them before [commit_tablet_job](https://github.com/apache/doris/blob/231d414a494e06aef5b3dfa43c02cc99b7d9eaad/be/src/cloud/cloud_cumulative_compaction.cpp#L473-L501) publishes the for mal rowset. Output IDs use per-BE counters and can sort below the reverse cursor. If both bitmap persistence and formal publication complete before the next checker page, that page sees the bitmap in its newer snapshot but compares it against the old cache, incorrectly returning `suspected_orphan`. This example concerns a fully published output, not a pending/tmp rowset. Refresh the cache on transaction renewal, or revalidate cache misses using the transaction that read the bitmap. Add a test publishing a compaction output between two successful bitmap pages; the existing retry test does not cover this transition. 2. **P2 — Bound repeated expiration while scanning one tablet's rowsets.** [checker.cpp:1509–1540](https://github.com/apache/doris/blob/231d414a494e06aef5b3dfa43c02cc99b7d9eaad/cloud/src/recycler/checker.cpp#L1509-L1540). All 128-rowset pages use the same transaction: [Transaction::full_range_get](https://github.com/apache/doris/blob/231d414a494e06aef5b3dfa43c02cc99b7d9eaad/cloud/src/meta-store/txn_kv.cpp#L721-L727) binds `opts.txn = this`. If a large tablet's full rowset scan repeatedly exceeds FDB's read-version window, the `TXN_TOO_OLD` branch clears every collected ID, retries the same bitmap inclusively, and refunds `scanned_kvs`. It makes no lasting progress and has no retry/time bound, so the connected synchronous request can loop indefinitely even with `max_scan_keys=1`. Bound retries/elapsed time and return an explicit incomplete or retryable outcome, or implement traversal that preserves progress and revalidates the candidate. This retains the single-tablet case of the [existing transaction-lifetime concern](https://github.com/apache/doris/pull/69264#discussion_r4311131937). The previously reported initial-cancellation issue is fixed: cancellation is checked before instance I/O and immediately after it. **Required Doris review checkpoints** | Checkpoint | Conclusion | | --- | --- | | Goal and test proof | Diagnostic flow is implemented; the applicable compaction/cache and repeated-expiry cases above remain uncovered. | | Scope, clarity and reuse | Focused endpoint/helper changes reuse route, key, error and JSON helpers. The additional aggregation replay test exercises an existing guard. | | Concurrency and locks | State is request-local; no new locks or lock-order edges. Under the no-ingestion premise, ongoing compaction can expose finding 1. | | Lifecycle and static initialization | Controller capture stays within synchronous execution; scoped transaction/iterator ownership is sound. No new static initialization dependency or ownership cycle found. | | Configuration | None added; parameters apply per request. | | Compatibility | Additive endpoint; no existing wire or storage format changes. No Thrift/protobuf definitions changed. | | Parallel paths | Background checking intentionally has broader pending/tmp exclusions; the new route explicitly checks v1 formal metadata. | | Special conditions and errors | Numeric validation, key decoding, cancellation, inclusive retry and exclusive continuation were traced. Finding 2 affects retry termination. | | Test coverage | Nine endpoint/scanner tests and one aggregation replay test inspected. Compaction publication across successful pages, repeated expiry and live HTTP/FDB execution remain gaps. | | Expected test results | No golden files changed; inspected expectations match the traced branches. No runtime pass is claimed. | | Observability | Responses expose status, count, exhaustion, elapsed time and candidate coordinates; cancellation is logged. | | Transactions, persistence and failover | Diagnostic is read-only; no new persisted state, EditLog or commit/replay protocol. Snapshot handling is covered by the findings. | | Data writes, atomicity and crashes | No new production writes/deletes. Recycler deletion ordering and query visibility are unchanged. | | FE–BE variable propagation | Not applicable. | | Performance and memory | Bitmap and rowset read batches are bounded; one tablet's IDs are retained. Repeated full-tablet work is finding 2. | | Other issues | No further substantiated issue found in the reviewed scope. | **Validation:** Static review of the complete pinned diff and the HTTP dispatch, FDB iterator, rowset publication/compaction and replay paths. Scope checked with `git diff BASE...HEAD` and `git log BASE..HEAD --oneline`; local HEAD matches the reviewed head and tracked files are unchanged. Per the user's instructions, no builds, formatting checks or tests were run. In particular, [MemTxnKv's full-range iterator](https://github.com/apache/doris/blob/231d414a494e06aef5b3dfa43c02cc99b7d9eaad/cloud/src/meta-store/mem_txn_kv.cpp#L890-L897) ignores batch limits, so the injected-expiry tests do not establish real FDB rowset-pagination behavior. Standards: 0 additional findings. Spec: 2 unresolved P2 findings if background compaction remains active; the repeated-expiry finding also applies when rowset metadata is otherwise quiescent. Recommend addressing the applicable cases before merge. -- 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]
