Static review of PR #68622 at head b5512c179739918df5e7599bf1b31a5d9ce36291: request changes. The updated 5,000-rowset KV baselines are consistent with the seeded cases on static inspection, and branch and total timings remain in the report after the elapsed-time failure gate is removed. The stated benchmark isolation goal is still unmet; one new P1 inline finding explains the default and explicit runner paths.

Critical checkpoints:

• Goal and proof: The workload and count baseline changes are present, but the benchmark still runs in the default Cloud unit-test binary. There is no test or runner change proving standalone selection. No build or test was run in this review.
• Scope and focus: The one-file edit is focused on the benchmark, but the omitted CMake target and runner changes are required for the PR's stated goal.
• Concurrency: Recycler worker tasks and packed-file references remain; no lock or shared-state implementation changed. I found no substantiated new race or lost packed-file coverage from the reduced count.
• Lifecycle and static initialization: Existing fixture setup, cleanup, thread-pool teardown, and config restoration remain. No cross-translation-unit initializer dependency was introduced.
• Configuration: The timing-tolerance environment setting was removed as intended. No new dynamic configuration was added.
• Compatibility: This is a test-only change; no function symbol, storage format, or rolling-upgrade contract changed.
• Parallel paths and conditions: The 14 named baselines still cover legacy, prepare, compacted, packed, V1/V2 bitmap, and mixed cases. The missing-result check remains; no distinct baseline or condition bug was substantiated.
• Tests and expected results: The fixture still checks recycler success, KV counts, remaining rowsets, packed-file cleanup, and bitmap cleanup. No .out result changed. The PR's reported measurements were not independently reproduced; this review is static only.
• Observability: Per-branch and total elapsed time and KV counts remain in the benchmark report; failure aggregation remains active for functional/count errors.
• Transactions, persistence, and writes: Only test seeding and recycler fixture KVs are affected. No production transaction, EditLog, persistence format, or FE/BE variable transfer changed.
• Performance and other issues: Halving the workload reduces benchmark work, but routine Cloud tests still execute it. No other substantiated performance or correctness issue remained after two review rounds and the final changed-file sweep.

The sole existing inline thread, comment #4130951007, asks “why?” about an earlier runner guard and does not report the current-head isolation failure. No already-reported P0/P1 inline finding still applies. There was no additional user-provided review focus.

Complete static review of PR #68622 at head b5512c1. The requested standalone benchmark target and default-run exclusion are still absent; existing [P1] inline comment 4142246387 remains applicable and is carried as a blocker without reposting. I found two additional [P2] false-failure paths in the revised exact KV baselines: concurrent first schema-cache fills and successful packed-file conflict retries. Both exact-count assumptions existed in the prior 10,000-rowset baselines; the changed 5,000-rowset entries retain them.

Critical checkpoints:

• Goal, scope, and proof: The changed file halves each branch workload, recalibrates all 14 KV baselines, removes elapsed-time failure thresholds, and retains timing output. The PR's stated standalone-target/default-discovery goal is unmet, as the existing P1 explains. The source edit is focused, but does not complete that goal. No test was executed in this review; the PR description reports compilation and shell syntax checks by the author and says unit tests were not run.
• Concurrency and lifecycle: Recycler scan pages submit independent worker jobs. The schema cache deliberately releases its mutex before the FDB read, and packed-file ref-count transactions retry conflicts; those paths substantiate the two new comments. I found no new lock-order, deadlock, static-initialization, ownership, or teardown change. The fixture still restores its modified config and stops its pools in teardown.
• Configuration and compatibility: The elapsed-tolerance environment setting is removed intentionally. No new configuration, dynamic setting, FE/BE variable, wire or storage format, or rolling-upgrade behavior is introduced.
• Parallel paths, conditions, and data correctness: Legacy, PREPARE mark/abort/direct, compacted, packed, and V1/V2 bitmap scenarios remain seeded and checked. The 5,000 count satisfies the packed-file divisibility and seed-batch conditions. Production recycler transaction and persistence code is unchanged; benchmark assertions still check progress, cleanup, and nonzero recycler returns.
• Test coverage and results: The final loop still requires every expected branch result and checks KV counts; teardown still fails accumulated errors and prints per-branch/total timings. Runtime correctness and the new numeric baselines are not independently proven because builds and tests are prohibited by this review invocation. The two inline comments identify legitimate schedules where exact counts reject successful recycling.
• Observability and performance: Timing and KV/rowset metrics remain visible in the report. Removing timing thresholds matches the stated policy; the smaller workload reduces work per branch. The benchmark still runs in default Cloud tests because it remains in recycler_test, as the existing P1 covers.
• Focus: No additional user-provided focus was specified. The changed-file sweep found no other unresolved point. Review completed after two full/risk-focused subagent rounds, independent candidate validation, duplicate fencing, and final sweep.

Existing P0/P1 findings confirmed for this head: #68622 (comment)

Static review of PR #68622 at head fa81bd32807ffe304fe8e201359a0b9aa29b7c98 (authoritative merge-base diff; one changed file). The 5,000-rowset workload and 14 ordinary-path KV baselines reconcile, and elapsed times remain in the printed benchmark report after the time-based failure thresholds are removed. The PR's stated goal of moving the benchmark out of default Cloud test runs is still unmet: cloud/test/CMakeLists.txt includes it in recycler_test, which the default runner executes, and there is no standalone recycler_benchmark_test binary for explicit selection. This is already reported by the still-applicable P1 inline comment (ID 4142246387), which is carried as an existing blocker rather than reposted.

The exact KV comparisons can also fail after successful recycling through a concurrent first schema-cache fetch or a packed-file conflict retry. Both are already reported P2 threads, so this review adds no duplicate inline comments. The author's reply about small data volume does not serialize the page workers: Doris uses a 10,000-key range limit, but its pinned FoundationDB versions map WANT_ALL to an 80 KB target (FDB 7.1.57 C binding); recycler page jobs run concurrently.

Critical checkpoint conclusions:

• Goal and proof: The changed benchmark constant, count table, and removed elapsed assertions implement the shorter and time-independent benchmark checks. The standalone/default-run goal remains blocked by existing P1. The benchmark test itself was not executed in this static review.
• Scope and clarity: The diff is confined to one benchmark file and its 14 recalibrated rows. It omits the CMake/runner changes required by the PR description.
• Concurrency and locks: recycle_rowsets submits scan-page jobs to its worker pool; the cache unlocks before fetching a schema, and packed-file ref-count transactions use FDB conflict/retry. Existing P2 threads cover variable measured counts. This diff adds no thread or lock, and no new lock-order issue was substantiated.
• Lifecycle and static initialization: Fixture config restoration, recycler reset, pool shutdown, and timing report remain intact; no new cross-translation-unit static dependency or ownership change appears.
• Configuration: The environment-controlled duration tolerance is removed as intended. No new configuration, dynamic update requirement, or restart behavior is introduced.
• Compatibility: No protocol, storage format, function symbol, FE/BE variable, or rolling-upgrade contract changes.
• Parallel paths and conditions: All 14 named measured branches still have baseline entries. Legacy, prepare/mark/abort, compacted, packed, V1/V2 bitmap, missing-schema, and mixed paths were traced. The 5,000-rowset static assertion and missing-result check are coherent with the new table.
• Test coverage and expected results: Rowset-count, bitmap-range, packed-file KV, and abort-ordering assertions remain. Seed commits are 2,000 + 2,000 + 1,000 rowsets. Per-key normalized counters explain the new ordinary-path values, including mixed 100000/20000/60000; the two already-reported concurrency cases make exact counts nondeterministic. No tests or builds were run by this review.
• Observability: measure still records elapsed time and KV metrics; TearDown prints each measured branch and totals. No new logging or metric path is needed for the benchmark-only diff.
• Transactions, persistence, and writes: The test still seeds and recycles FDB metadata, but this diff changes no production transaction, EditLog, object deletion, crash recovery, or visible-version behavior. Packed-file retry and cache-fill measurement risks are covered by the existing P2 threads.
• Performance and resource use: Halving seeded rowsets reduces benchmark work in principle; no runtime gain is claimed without execution. Default-run cost remains until the existing P1 is fixed. No new memory allocation, status handling, or data correctness issue was substantiated.

Review completion: one convergence round completed with two full-review subagents and two risk-focused subagents, all reporting NO_NEW_VALUABLE_FINDINGS after rechecks. Every candidate was verified as an existing-thread duplicate or dismissed with code evidence. Final changed-file and unresolved-candidate sweeps found no additional distinct issue. User focus file specified no additional focus. Static review only; no build, test, or source edit was performed.

Existing P0/P1 findings confirmed for this head: #68622 (comment)

LGTM