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? |
|
mo-bot review · Commit: Review completed. Read the assessment. |
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: 73bcb140154f74b7c490dd8dddd77a20c52e923a
Assessment: approve
The complete delta safely removes two unconsumed test helpers and an unused import. Existing fixture setup, cleanup, schema conversion, HNSW registration and test assertions remain intact. Removal is preferable to retaining wrappers without callers. The benefit is reduced maintenance; no runtime performance improvement is claimed. No actionable defects or substantive discussion disputes remain.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1586-7a01-9000-d45e20995cf8 · slot 1 · generation 1
Validation:
-
Parsed all 6,824 tracked Go files, checked import-bound references, and confirmed retained util.go declarations match the base structurally.
-
Full testutil package passed: three top-level tests and two subtests. TestRelationExists passed, covering existing/missing relations, invalid-tenant errors and fixture cleanup.
-
Changed-file formatting, diff whitespace and go vet for testutil and its engine consumer passed.
-
Isolation preflight passed. All 192 native inputs matched the reused build; repository provenance verification passed before execution.
-
Verified exact-head command/source/exit receipts against retained logs. Saved review-evidence.json, review-scope.json and complete discussion-assessment.json under /home/xupeng/.local/state/mo-bot/jobs/4a3f4e68276ec7c1530cf9c6/attempt-1. Final evidence audit passed; source remained unchanged.
Limitations:
-
Frozen CI was pending and local_validation was empty. Author-reported configured lint and cross-PR merge checks were not independently verified.
-
Execution covered Linux/amd64 CPU. No reachable behavior changed to warrant additional race, GPU, SQL BVT or performance runs.
-
The raw linked tracking issue was absent; its engineering-quality scope is documented in frozen source. Approval covers this retirement, not broader issue closure or unknown out-of-tree Go consumers.
## What type of PR is this? - [ ] API-change - [ ] BUG - [ ] Improvement - [ ] Documentation - [ ] Feature - [ ] Test and CI - [x] Code Refactoring ## Which issue(s) this PR fixes: Refs #29249 ## What this PR does / why we need it: Remove unused helpers, duplicate bookkeeping and transient state whose consumers have already been retired. This consolidates the 19 closed cleanup PRs into one reviewable change on current main; each affected file matches its original published PR, with no additional cleanup. The cleanup covers TAE task/logstore/merge/RPC helpers, HAKeeper replica views, obsolete ETL writer and path state, batch/vector constructors, RPC client bookkeeping, metrics/CDC/fulltext state, MongoScan, message receivers, fixture helpers and interactive viewport state. Retained execution, ownership, cancellation, parser/error, replay, plugin registration and MySQL driver registration paths remain intact. Supersedes #31325, #31327, #31329, #31331, #31332, #31333, #31334, #31335, #31336, #31337, #31339, #31340, #31341, #31342, #31343, #31344, #31345, #31346 and #31347. Already merged #31323/#31324 and Python SDK #31322 are excluded. Validation on the combined change: all 30 changed owning packages built; 28 ran tests, with 2,066 top-level / 4,580 named cases passing and two existing external-data ETL tests skipped. The two support packages have no tests. Existing component-level race, before/after probes and consumer checks were reviewed for reuse. Formatting, go vet and incremental golangci-lint passed; forced all-four-analyzer molint reported six findings already reproduced at the baseline, with no new findings. These are local focused checks, not a full CI or GPU run.
What type of PR is this?
Which issue(s) this PR fixes:
Refs #29249
What this PR does / why we need it:
Retire two orphaned engine-test helpers and the unused assert import:
MakeDefaultTestPath: its manual file-service overrides were removed when the existing engine fixture took over setup in perf(optools): bound race UT critical path safely #29760. Keep the liveGetDefaultTestPathand fixture ownership unchanged.EngineDefAddIndex: its sole caller was the unused HNSW fixture helper removed in refactor: retire unused mocks, helpers and obsolete scaffolding #31011. Keep real schema conversion, index plugins, ISCP/CDC and optional capabilities unchanged.No maintained test case, body or assertion changes. No dependency metadata changes.
Validation (Go 1.27.1, controlled native wrapper):
TestRelationExistspasses, covering existing/missing relations and invalid-tenant errors.