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? |
|
已修复这轮 CI 的共同编译阻断,保留本 PR 原有 SQL 修复与全部测试门禁。 根因不是本 PR 的 SQL 专项失败:Main 的 #30029 引入 Gossip fixture 调用后,#29978 将 最小修复仅改一个测试文件:先获取现有 本地验证(50,Linux/amd64,Go 1.27.1):
同一修复已同步 #30091 和 #30092。推送会触发完整 CI;本地验证不冒充新的远端 ARM64 SCA/完整覆盖率已绿,最终以新一轮 CI 结果为准。 |
LeftHandCold
left a comment
There was a problem hiding this comment.
深度 review 结论:没有发现本 PR 引入的阻断级问题,代码层面建议 APPROVE。当前认证账号与 PR 作者相同,GitHub 不允许 self-approve,因此以 COMMENT 记录结论,不冒充正式审批。
范围:main / merge-base 21f19ae063 → 最新 HEAD 7a6b891311,完整审查 3 个文件;审查中发现 HEAD 更新后,已补看端口 fixture 修复。
设计与正确性
- #30090 的修复落在正确的入口:RC
CREATE OR REPLACE VIEW在拿数据库锁前进入已有 C → G → D 准入。没有放宽 FastFail、新增重试或另造生命周期状态。 - 真实保留前序数据锁的事务仍由现有准入逻辑选择 FastFail;新鲜事务可以等待 SNAPSHOT gate,而不是被本语句提前取得的数据库锁误判。
- 保留后面的
lockMoDatabase和嵌套 DROP 不是应当直接删除的重复代码:前者保留普通锁的定义变更检查,后者仍负责 DROP 的对象身份/域复核。提前取得准入不能替代这些检查。 - 普通 CREATE VIEW、非 RC 路径未改。额外锁请求、catalog 校验和 snapshot barrier 限于 RC 的 CREATE OR REPLACE VIEW 语句(包括目标尚不存在的情况),不进入 SELECT/DML 热路径;不声称该 DDL 零开销。
- 最新 Gossip 增量正确复用
testPortRange()和现有有界 allocator,范围解析失败在配置创建前返回;不改变产品端口、重试预算或 CI 门禁。
| 审计 | 结论 |
|---|---|
| Q1 所有权/清理 | 准入锁继续归同一事务,沿已有 commit/rollback/cancel 释放;取消测试在仍持有 blocker 时确认服务端事务消失,再验证旧视图未变。 |
| Q2 等待/退出 | 恢复 C → G → D 顺序,保留 retained-lock FastFail;锁等待继续使用现有 context/lock-wait budget。测试采用真实入队观察点,不靠 sleep 判定阶段。 |
| Q3 增长/资源 | 无新增常驻 goroutine、缓存或重试队列;新增端口测试只创建两节点配置,复用已有清理。 |
验证与边界
- 已核对 旧 HEAD CI run 38346948671:实际 checkout
c45ff22405;Linux/amd64 race 模式下TestIssue30090ReplaceViewLifecycleOrder及 release/cancel/prepared-reader/retained-lock 四个子场景均 PASS,顶层用例 2.33s;pkg/sql/compilePASS 18.193s,issues 四个执行批次均 exit 0。两个已启用 BVT job 也通过。 c45ff22405→7a6b891311仅改变 Logservice 的_test.go;上述 SQL 生产代码、测试、依赖和构建配置均未变,该 SQL/race 证据可复用。- 已读取 50 上此次修复的原始日志和验证记录:Logservice 全包 PASS 196.285s、新端口检查 race 100 次 PASS、六节点 Gossip consumer race 6 次 PASS;最新 head 双 CN 视图专项复跑 PASS 8.656s。也核对了最新源码 hash、Go 1.27.1 gofmt 和 diff-check。
- 我额外发起的单任务 race 复跑限制为 2 核 / 6 GiB / 5 分钟;到上限时未产生测试执行结果,任务已停止回收,不把这次复跑计作 PASS,也不把它当成产品测试失败。没有运行全套本地 UT。
- 旧 CI 的
randomPorts.allocate参数不匹配属于独立的测试编译问题,最新增量已修正;最新完整 CI 38361983307 尚未完成,本结论不代表所有合并门禁已绿,也不替代完整 nightly 验收。
当前没有待处理的 inline review thread。没有需要以 speculative blocker、代码风格或额外抽象为由要求修改的项。
fb3f94d to
76c1ab9
Compare
|
Updated in
Final source validation: whole compile normal/race PASS (10.704/12.754s), whole issues normal/race PASS (265.380/644.282s), no failures/skips in either issues mode. Five real public subcases, three same-process race repetitions, updated barrier/reconnect normal/race, retained listener test and configured incremental SCA all passed. Source fingerprints are unchanged by committing. Required actual gpt-6.1-sol/xhigh design and final overall gates PASS. New remote CI has been triggered by the push; its completion is not claimed. No merge or review dismissal/approval was performed. Added admission work remains bounded to pessimistic RC replacement DDL; ordinary SELECT/DML paths are unchanged. PR body now records the final scope, validation and limitations. |
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: 76c1ab96c0bd1c570fe61a8f6885c372edb7b8cd
Assessment: approve
Reviewed the complete three-file change and lifecycle ownership chain. Early admission fixes fresh view replacement while preserving retained-lock FastFail, cancellation cleanup, nested DROP validation and error 1046. The previous duplicate-test concern is resolved; no actionable findings remain.
Model: gpt-6-astra / xhigh
Session: 01a12d8f-8e48-7622-ad17-b605f08450ce · slot 1 · generation 0
Validation:
-
Verified clean frozen HEAD and matching patch; the supplied patch only omits its final newline.
-
Built native prerequisites from matching exported inputs through make cgo; retained input/artifact hashes and build receipt. Isolation preflight passed.
-
Normal TestScope_CreateView and the two-CN TestIssue30090ReplaceViewLifecycleOrder passed, covering missing database, gate release/cancellation, subsequent replacement, prepared-reader rebinding and retained-lock rejection. Receipt: checks/1791929695133278154-2.json; log: focused-tests.log.
-
Race regression passed (10.73s), followed by two same-process repetitions; complete pkg/sql/compile passed under race (11.107s). No failed or skipped tests. Receipt: checks/1791930212238638345-2.json; logs: focused-race.log, focused-race-stress.log, compile-package-race.log.
-
Commands, source fingerprints, terminal receipts and log hashes reconciled in /home/xupeng/.local/state/mo-bot/jobs/a862e6697d35ecbcdde54466/attempt-1/review-evidence-audit.json; final reconciliation receipt: checks/1791930250595323085-2.json.
Limitations:
-
No full-repository or deployed-cluster suite was run. Author-reported broader checks lacked retained execution artifacts and were not counted as independent passes.
-
No quantitative latency benchmark was performed; added admission work is confined to pessimistic RC view replacement.
Merge Queue Status
This pull request spent 9 minutes 52 seconds in the queue, with no time running CI. ReasonPull request #30092 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. Waiting for any of
All conditions
Tick the box to put this pull request back in the merge queue (same as
|
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #30090.
What this PR does / why we need it:
A fresh pessimistic RC
CREATE OR REPLACE VIEWpreviously took the database lock before nested DROP lifecycle admission. That manufactured retained ownership and caused FastFail 20706 behind a SNAPSHOT gate instead of waiting. The replacement now enters the existing C/G/D admission before taking the ordinary database lock. Real retained-lock transactions still FastFail; nested DROP retains domain/identity revalidation and definition-change fences.The replacement admission also preserves the existing no-default-database error: constant views without USE return MySQL 1046 before admission or engine/lock effects. The regression was reproduced on the previous PR implementation, where ordinary CREATE returned 1046 but OR REPLACE returned an internal lifecycle error.
Scope: 16 production lines, 215 test lines, no standalone documentation changes. Deleted the PR's redundant six-port test: the existing test already covers nine rendered listener addresses, allocator range and uniqueness. The integration test registers holder rollback immediately after BeginTx so assertion failures cannot bypass cleanup. No lock-service policy, retry, timeout, catalog format, or new admission state is introduced.
Validation
Rebased without conflicts onto main
8aaa8ae9e6; published head76c1ab96c0. Local Go 1.27.1 / darwin-arm64; repository CGo wrapper and freshly built source-matched native artifacts. ONNX cache checksum matches the repository's required artifact.Prior-head CI and author Shanghai50 evidence are historical, not new-head passes. New remote CI is not claimed passed before it executes.
Performance and safety
Additional work is bounded to pessimistic RC replacement DDL, including a registry/frontier admission pass. Plain CREATE VIEW and non-RC routing and SELECT/DML hot paths are unchanged. Replacement DDL is not claimed to have zero overhead or a measured throughput improvement.
Locks stay transaction-owned through existing commit/rollback/cancellation. Tests observe a real queued waiter and cancelled server ownership disappearance, rather than using sleeps to establish phases. No worker, cache, queue, retry loop or unbounded retained state is added. Full TKE nightly acceptance is not claimed. This is separate from temporary DELETE (#30089/#30091) and prepared pruning (#29661).