Repository navigation
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
LeftHandCold
left a comment
LeftHandCold
left a comment
There was a problem hiding this comment.
基于最新 HEAD b93970e7e92a7392b0181a6d2f587334d6689491 完成深度 review：代码审查通过，未发现需要 Request changes 的新增正确性、性能回归或生命周期问题。
重点结论：
- 设计与职责合理：目录可见性仍由 CatalogCache 负责，统计层仅按 account/database 分组并恢复输入顺序，单条与批量共用 tablePair 元数据投影。没有引入第二套持久索引、后台任务、重试状态机，也没有放松鉴权/logtail 屏障。
- 结果语义保持一致：核对了快照边界、同名删建/替换 ID、同时间戳 insert/delete 顺序、rename、跨库/租户、重复/缺失 ID、view 排除、PK sequence 和 snapshot 传递；批量遍历遇到 tombstone 或未请求的替换 ID 仍会屏蔽同名旧版本。统计入口和 ISCP 使用的 exported callback 路径均已追踪。
- 锁与资源：沿用现有 BTree 读锁，回调内没有新增 I/O、嵌套锁或外部等待；GC/目录版本的存活规则未改动。新增 maps/slices 为随请求规模线性增长的调用内临时对象，不是长期缓存。单 ID 保留原路径，重复扫描由“每个 ID 一次”降为“每组 account/database 一次”。
- 验证证据：核对了作者留存的两个 owning packages 的 normal/race、focused race、并发 race ×100、真实 engine smoke 和有限工作量对照的成功终态日志；四个交付文件在 authored revision
d32ae8663d与当前 HEAD 的 Git blobs 完全一致，相关依赖/原生源码未被合入的 main 改变。这些属于复用的作者执行证据，不声称本轮重新运行过完整生产包。 - 本轮补充验证：轻量 harness 使用实际 tidwall/btree，并校验 10 个抽取函数与当前 HEAD 的 AST 一致；4 个测试通过，包括 rename/logical-TS 边界、64 个确定性历史种子（186,080 个 scalar/batch/独立模型对比）及 512 个分组结果对比。它是机制对照验证，不替代 production-linked UT、race 或 ANLI 回放。未重复执行大规模 native build。
性能对照的旧/新路径原始日志支持本次修复的针对性：同样的 45,559 个名字 × 8 个版本、4,000 个请求、1,000 行目录写入下，完整写入耗时约 18.826s → 12.529ms；这是受控合成场景，不能外推为业务 QPS 提升比例。
验收边界：当前 merged-head CI 仍有运行/排队项，不能描述为全部通过；#29965 仍应保留同负载 Family/ANLI 验收，不能仅凭组件实验宣布所有 Proxy 20508 都已根治。这个边界已在 PR 中明确，不构成本次局部修复的额外代码 blocker。
当前认证账号与 PR 作者相同，GitHub 不允许自 Approve，因此用 COMMENT 记录上述通过结论；正式 Approve 需另一位 reviewer。
|
Fixed the SCA failure in 54d8f00. The only reported violation was prealloc in TestGetChangedTableListBatchLookupConsumer: gotIDs is now allocated with length 0 and capacity len(pairs). No production code, assertions, or lint configuration changed. Validation on shanghai50: the owning disttae package passes the exact prealloc v1.1.0 analyzer used by golangci-lint v2.14.0; TestGetChangedTableListBatchLookupConsumer and its CheckChanged / CollectChanged subtests pass via the repository CGo wrapper. gofmt and git diff --check are clean. These targeted checks reused the existing isolated base-plus-overlay harness; they are not a claim that the complete new GitHub CI run has passed. The pushed commit will receive fresh CI. |
|
mo-bot review · Commit: Assessment: request_changes Reviewed the complete four-file delta. Production batching preserves catalog visibility, ordering, tenant boundaries and existing execution ownership. One low-priority test resource leak remains. Model: Session:
Validation:
Limitations:
|
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: 54d8f0080425d5a0aaf45a668d307b68950dbce5
Assessment: request_changes
Reviewed the complete four-file delta. Production batching preserves catalog visibility, ordering, tenant boundaries and existing execution ownership. One low-priority test resource leak remains.
Model: gpt-6-astra / xhigh
Session: 01a12d8f-7dea-77c2-a565-38d45b5564e4 · slot 0 · generation 0
- P3
pkg/vm/engine/disttae/mo_table_stats_lookup_test.go:39: Both new statistics tests call newStatsLookupEngine; each invocation creates a globally registered MPool.
Impact: The pool and its ownership metadata remain reachable after test cleanup. Repeated runs accumulate retained pools even though the byte-count assertion passes.
Evidence: MustNewZero delegates to NewMPool, which stores the pool in globalPools (pkg/common/mpool/mpool.go:768). DeleteMPool removes that registration at line 945, but this fixture only checks CurrNB and never calls it. Register DeleteMPool cleanup that also executes if the assertion fails.
Validation:
-
Reviewed all changed implementation, tests and benchmarks; traced statistics, ISCP callbacks, TN response construction, snapshot visibility, concurrent writers and GC.
-
Recovered earlier-attempt review work and confirmed identical patches. Latest evidence differs only in repository issue counts.
-
Reused exact-head successful build, Ubuntu UT, coverage, arm64 SCA, proxy BVT and pessimistic multi-CN BVT statuses.
-
Verified production lookup code and cache tests are unchanged from the author's tested revision; reused reported normal/race, focused stress, engine-smoke and performance evidence. The subsequent consumer-test edit only preallocates result IDs.
-
Confirmed the fixture leak through allocation/destruction ownership tracing. Source remains unchanged and git diff --check passes. Detailed audit saved in attempt-3/review-audit.json.
Limitations:
-
No native tests or benchmarks were rerun locally; execution evidence comes from frozen CI statuses and author/reviewer reports, without raw logs in the supplied bundle.
-
Full linked-issue text was unavailable. The PR explicitly leaves broader #29965 Family/ANLI acceptance open; component performance results do not establish complete incident resolution or application-wide regression freedom.
|
Addressed the requested change in 0da8dbf. The finding is valid: MustNewZero -> NewMPool registers the fixture pool in globalPools. CurrNB()==0 proves the allocated bytes were freed, but does not unregister the pool or release that global reference. Both statistics tests shared this missing cleanup. The existing t.Cleanup now defers mpool.DeleteMPool(mp) before require.Zero(t, mp.CurrNB()). This preserves the leak assertion before destruction and guarantees pool deregistration even if the assertion calls FailNow. Cleanup still runs after the test and its subtests; no production lookup, authentication, GC, or allocation policy changed. Validation on shanghai50 using the existing isolated native/CGo harness and updated test overlay:
Prior CI passed on 54d8f00; new-head CI is separate and still pending. Please re-review this focused test-lifecycle correction. |
|
mo-bot review · Commit: Assessment: approve Reviewed the complete four-file delta. Batching preserves catalog visibility, tenant boundaries, ordering and metadata while reducing repeated scans. The previously reported fixture-pool leak is fixed, including assertion-failure cleanup. No actionable findings remain within the scoped contract. Model: Session: Validation:
Limitations:
|
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: 0da8dbfd612321826dfa4080f82db52c70a91d53
Assessment: approve
Reviewed the complete four-file delta. Batching preserves catalog visibility, tenant boundaries, ordering and metadata while reducing repeated scans. The previously reported fixture-pool leak is fixed, including assertion-failure cleanup. No actionable findings remain within the scoped contract.
Model: gpt-6-astra / xhigh
Session: 01a12d8f-7dea-77c2-a565-38d45b5564e4 · slot 0 · generation 0
Validation:
-
Traced scalar and batch lookup, tombstones, replacements, snapshot boundaries, concurrent writers, GC, statistics callers, TN responses and ISCP callbacks.
-
Verified MustNewZero registration is paired with deferred DeleteMPool after the byte-count assertion and completion of subtests.
-
Confirmed production files and cache tests match the author's tested revision; the latest commit changes only fixture cleanup.
-
Reused reported owning-package normal/race tests, focused race stress, real-engine SQL/TN smoke tests, controlled performance comparisons and current-fixture normal/race results.
-
Frozen exact-head CI records successful build, Ubuntu UT, coverage, arm64 SCA, Proxy BVT and pessimistic multi-CN BVT.
-
Verified frozen HEAD/base, supplied-patch equality, clean source status, formatting and git diff --check. Saved review-audit.json and static-validation.json in the attempt directory.
Limitations:
-
No native tests or benchmarks were rerun locally. Execution evidence comes from frozen CI statuses and author/reviewer reports; raw execution logs are absent from the supplied bundle.
-
Full linked-issue text was unavailable. The PR explicitly leaves broader #29965 Family/ANLI acceptance open; component measurements do not establish complete incident resolution or application-wide regression freedom.
Merge Queue Status
This pull request spent 27 minutes 29 seconds in the queue, with no time running CI. ReasonPull request #29983 has been dequeued Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Required conditions to merge
Tick the box to put this pull request back in the merge queue (same as
|
This branch was successfully deployed
3 participants
What type of PR is this?
Which issue(s) this PR fixes:
Related to #29965: fixes the repeated catalog-ID scan amplification identified
in its statistics preparation path. Keep the incident open for Family
acceptance rather than automatically declaring every observed handshake
failure resolved by a component experiment.
What this PR does / why we need it:
Root Cause
getChangedTableListresolves each returned table ID separately. The catalogBTree is ordered by account, database, name and descending timestamp, not by
table ID. Consequently M requests can traverse N retained name/version entries
M times, including full-scope scans for already-dropped IDs.
Each traversal holds the BTree read lock. The logtail receiver needs its write
lock for each catalog update. Writer preference prevents permanent starvation,
but repeated short scans can interleave with many catalog writes and delay
completion of a whole update batch. Authentication read barriers behind that
work then wait; weakening those barriers would break catalog consistency.
Incident evidence in #29965 contains both statistics scans and receiver write
lock waits, with authentication blocked on read barriers. Native histogram
samples add a discriminating observation: in a 30-second affected interval,
4,482 completed catalog-cache update observations accumulated 29.740 seconds;
each individual observation was at most the 45.562ms histogram bucket boundary.
This is cumulative contention, not proof of one continuously held 30s lock.
Scoped Change
metadata; preserve tombstones, replacement identities and same-TS ordering.
worker, retry, timeout, protocol, authentication, GC or routing changes.
tablePair storage only for accepted tables, not every missing ID.
Validation
Base: main
02a2823c2007d5e4fb4ada148e93743df337551a.Tests run on shanghai50, Linux amd64, Go 1.27.1, repository CGo test wrapper,
verified matching native artifacts, four CPU affinity slots.
After PR creation, Mergify merged main
7af631bdf7(#29862, Sirius acceptancecampaign) into this branch as
b93970e7e9. All four delivery files are byte-for-byteunchanged. The local/50 results below are from the authored
d32ae8663drevision;merged-head GitHub CI is a separate, currently pending check.
Focused snapshot and changed-table consumer tests passed. The consumer test
uses a real catalog cache and transaction timestamp waiter, exercises both
CheckChanged and CollectChanged through the exported callback API, and covers
zero-TS requests, ordering, duplicates, views and missing IDs.
disttae/cachepackagedisttaepackageTestGetTablesByIdAndTimeConcurrentWriterAndGC, race x100TestMoTableStatsMoCtlandTestHandleGetChangedListengine testsThe focused concurrency test's first race run rounded to 0s, so the repository
adaptive rule selected 100 repetitions. Full packages were not repeat-stressed.
The engine tests use real engine/SQL execution and TN changed-table collection;
they are smoke coverage, not a full Proxy/ANLI reproduction. New UTs use tiny
catalog fixtures and deterministic start synchronization, not timing assertions.
Equal finite-work diagnostic (not a CI timing assertion): one scope with
45,559 names and eight versions, 4,000 requested IDs (90 visible), and a fixed
1,000-row InsertTable plus DeleteTable workload:
The old reader itself took 38.418s versus 10.849ms for the new batch. Returned
IDs and visible versions are checked in both arms. No injected lock holds or
sleeps are used. The synthetic retained catalog is not claimed to reproduce the
exact incident scope distribution or all 512/508 ANLI failures.
Permanent microbenchmark, 4,096 names x four versions and 64 missing IDs:
8.425ms / 23,040B / 65 allocations becomes 0.155ms / 3,208B / 5 allocations.
Singleton measurements are approximately 132/131us with the same 360B and two
allocations. These measure catalog lookup work, not application QPS.
Risk And Acceptance Boundary
The existing snapshot/name visibility algorithm and tree synchronization remain
unchanged; no locks are released mid-traversal. Ordinary scalar readers, auth
barriers, data caches and vector/SQL execution paths are not modified. This
patch does not promise bounded latency for an arbitrarily large single scope,
eliminate every other catalog reader, or replace same-workload Daily acceptance.