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? |
…30075) ## What type of PR is this? - [x] Improvement - [x] Test and CI ## Which issue(s) this PR fixes: Closes #30064 Fixes #30078 ## What this PR does / why we need it: Move SDK validation into the repository-root Actions directory. Replace undiscovered nested workflows and missing/mismatched requirement paths with installed-package offline and distribution checks for Python 3.10/3.12 with SQLAlchemy 1.4/2.0, and Python 3.11/3.12 with SQLAlchemy 2.1. Package installation registers the dialect entry point; SQLAlchemy's asyncio extra supplies the required runtime. Include offline analytics cases when that directory is present. Add an explicitly invoked online job for a configured isolated self-hosted runner/environment; failures are not suppressed and JUnit summaries expose skipped cases. No runner, environment or credential is provisioned here. SDK_CI.md documents prerequisites, and this selected matrix does not certify advertised Python 3.8/3.9 support. Adapt the existing reserved-metadata test to SQLAlchemy 2.1's warning contract while retaining the exception assertion for 1.4/2.0; no blanket xfail/skip is added. ## Validation - Full installed-package offline suite: 977 passed plus 86 unittest subtests independently on SQLAlchemy 1.4.54, 2.0.54 and 2.1.4. - Parsed workflow YAML; verified PR/push/dispatch triggers and all matrix entries; bash -n passed for each embedded run script. - No continue-on-error remains in the new workflow. Online execution is only an explicit dispatch against a configured test deployment. - Local validation used Python 3.12; GitHub CI now independently passed all six supported Python 3.10/3.11/3.12 matrix cells; the local run alone was not used to claim those runtimes. - Wheel/sdist checks are part of the workflow; local wheel building is separately verified. All reported integration runs use the isolated local MatrixOne source build `cd8d5abf9665a97d13e50b04840fb74b877fd339`, synthetic data and (for TLS) local test certificates. They do not certify a production deployment or a released server binary. These are draft PRs for review; no merge is performed. The four fixes are tested together as well as independently, and integration with #30054 is being verified. GitHub CI status is separate from the recorded local results. ### Combined integration validation All four changes plus the analytics implementation from #30054 were tested together: **1258 passed, 86 unittest subtests passed, zero failures/errors/skips** in 38.38 seconds (JUnit counts 1344 including the subtests). This includes the installed-package offline suite, all 192 analytics cases with S3/two-CN prerequisites, and 30 new native regression cases. Bulk tests use two local plaintext CNs; strict certificate/no-downgrade cases use a separate dedicated TLS CN. The earlier TLS stress failure remains recorded and linked to the existing server issue. This is a local integration result, not GitHub CI completion. ### GitHub matrix correction The first run passed five matrix cells; Python 3.10 / SQLAlchemy 2.1 failed during dependency installation because SQLAlchemy 2.1 declares Requires-Python >=3.11. Commit da01818 replaces that unsupported cell with Python 3.11 / SQLAlchemy 2.1, preserving six valid cells. The failed first run is retained: https://github.com/matrixorigin/matrixone/actions/runs/38336849870 . All six supported SDK matrix jobs passed in the corrected run: https://github.com/matrixorigin/matrixone/actions/runs/38338130924 . The manually invoked online job was not run; local online evidence is reported separately. ### SDK-only CI routing Extend the existing trusted planner with SDK-only routing instead of a second classifier. `clients/python/**` plus ordinary docs runs the SDK checks and omits kernel build, UT, coverage, BVT, upgrade and utils. Mixed SDK/kernel changes retain both suites. SDK CI control changes, force-full and untrustworthy file lists require full kernel plus SDK checks; rename impact follows both paths. The root workflow is the sole reusable automatic SDK PR owner, called with contents-read permissions and no inherited secrets. Offline jobs check out immutable fork-head code on hosted runners without persisted credentials. Manual credentialed online execution stays separate. Distinct concurrency groups, bounded offline execution and per-run/attempt/cell artifacts avoid duplicate ownership and identity collisions. CI Required rejects missing or unsuccessful required SDK results. Routing evidence: 31 Node contract tests, incremental workflow/static checks and fatal Python lint passed. The six-cell SDK run linked above remains evidence for its exact parent revision; it is not a live qualification of the modified routing. ### Documentation-only CI routing Recognize ordinary Markdown (`.md`) and reStructuredText (`.rst`) documentation throughout the repository, with case-insensitive extensions, instead of requiring one of four root names or a path under `docs/`. Build guides, package READMEs, skill documentation and RST pages now use the existing documentation whitespace check and metadata/size-label job, without kernel build, UT, coverage, BVT or upgrade execution. Keep the existing classifier as the sole owner. SDK documentation retains SDK checks because its README is consumed by packaging. Exact `testdata/` and `fixtures/` directory segments retain their input owner (SDK within `clients/python/`, full kernel otherwise); lookalike names remain ordinary docs. Executable MDX, text inputs, scripts/configuration and images outside `docs/` are not exempted. Mixed changes and both rename paths retain their required suites, and untrustworthy file lists or forced-full retain conservative full+SDK behavior. Validation: all 31 owning Node contract tests passed with zero failures/skips/cancellations; 251 composed checks execute the actual preflight script, classifier, workflow admission conditions and required-result verifier. Required unsuccessful/missing results are rejected. A real committed Git diff with a sparse `docs` checkout accepts a clean package README absent from the worktree and rejects both trailing whitespace and blank-at-EOF damage. Node syntax and incremental whitespace checks passed. The exact-head hosted [CI scope contract](https://github.com/matrixorigin/matrixone/actions/runs/38358226404) also passed. Existing fixtures/helpers are reused; no new workflow, API scan, stored state or server/SDK runtime work is introduced. The required design and final overall review gates completed before publication. No full kernel CI was run for this classifier follow-up; its automatically triggered full run is cancelled under the requested scope, while the lightweight contract workflow is retained. Live routing becomes effective only after deployment to the trusted target branch; this PR is not merged here. ### Online review fixes and current validation All actual default/hardcoded online connection entry points now consume the existing OnlineTestConfig snapshot, including partial-default JSON setup and duplicated Client constructor mappings. Fixture-created Pinecone databases preserve their explicit database selection while using configured host/auth; ORM bootstrap and cleanup use the configured base database, with its worker using the fixture database. No SDK production code or TP execution path is changed. The small kwargs adapter creates a fresh mapping only during test connection setup, with no new configuration store, cache or retries. Move four pure configuration oracles to the offline layer. Scoped pytest monkeypatch contexts restore both environment values and key absence after success, assertion failure and constructor failure; unused saved fields and manual cleanup are removed. - **Current-code full offline suite:** 1035 tests plus 86 unittest subtests passed separately with SQLAlchemy 1.4.54 and 2.0.44 on Python 3.10.18. - **Focused strict-mode regressions:** 58 passed, covering every migrated real constructor/method/unwrapped fixture under remote/default configuration, fixture database ownership through ORM cleanup, connection failure without fallback, exact environment restoration and subsequent consumer/configuration generations. These probes stop at the connection boundary and make no database connection. - **Incremental SCA:** syntax, configured Black/isort with exact pre-fix baseline comparison, fatal flake8 and whitespace checks passed. New tests are formatted; no new format violations are introduced into existing files. - Required gpt-6.1-sol/xhigh design and final overall review gates completed before publishing the reviewed content. The historical six-cell offline result does not validate these new/moved tests; the fresh local results above do. Matrix/dependency definitions and packaged SDK production contents are unchanged. Credentialed online SQL execution and live SDK-only routing after trusted-main deployment remain pending rollout qualification. Go race/kernel/CGo/BVT checks are inapplicable; deterministic scoped restoration/failure/sequence tests cover the affected Python state, for which race instrumentation is unsupported. Full kernel CI automatically triggered by this update is cancelled per the requested scope and is not reported as passing. The logservice/ISCP records below are retained from parent `dcf542fe5d41985edfdceaad592acbf096800f3e`. Their kernel CI status is separate from this Python-only follow-up; they are not replaced by the Python evidence above. ### Shared logservice CI compilation repair The original SCA and UT coverage failures shared the official-main defect #30047: the gossip fixture called an obsolete allocator signature. This branch includes the small, tested repair from #30083 so its head can compile without waiting for upstream rollout. The original SDK implementation is unchanged. Validation: clean-main compiler reproduction, fixed Go vet, existing port/gossip collision/cancellation/rollback tests passed (95.107s), and the new three-node range/uniqueness regression passed 20 times (0.082s). All tests retain their assertions. The separate ISCP failure from an earlier #30075 coverage run is being investigated; its isolated native reproduction passed on latest main (34.828s). New full CI results remain pending. This follow-up retains the SDK routing commit 93b33e8. The new head also contains the logservice test repair, so the latest kernel CI triggered by this follow-up validates that Go change; earlier cancelled runs are not counted as passes. ### ISCP failure reproduction follow-up The retained earlier coverage artifact shows `TestIssue29400DropDatabaseRCReclaimFailureRollsBackWholeDatabase` timing out at the initial ISCP-consumer readiness assertion (line 1091), with preceding lease-reader errors `unexpected rows count: 8192`. The isolated scenario passes on clean latest-main source (34.828s), and three consecutive runs also pass using `-covermode=set` with the exact cross-package instrumentation scope from that CI artifact (76.768s). This exercises instrumented production code; it is distinct from an earlier test-package-only coverage attempt whose output explicitly had no statements. These passes do not reproduce the full-suite concurrency/order conditions, and do not establish that the earlier failure is fixed. No ISCP assertion, timeout, lease uniqueness check or production code has been changed. The new full CI run remains authoritative. --------- Co-authored-by: XuPeng-SH <xupeng3112@163.com>
|
mo-bot review · Commit: Assessment: approve Reviewed the complete two-file delta and allocator, fixture lifecycle and consumer paths. The signature repair reuses the authoritative listener range and allocator, preserves error cleanup, and adds meaningful endpoint coverage. No actionable defects remain. 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: 0e6a7d897c8eedc28815ff61076852ce4556a57d
Assessment: approve
Reviewed the complete two-file delta and allocator, fixture lifecycle and consumer paths. The signature repair reuses the authoritative listener range and allocator, preserves error cleanup, and adds meaningful endpoint coverage. No actionable defects remain.
Model: gpt-6-astra / xhigh
Session: 01a12d8f-7dea-77c2-a565-38d45b5564e4 · slot 0 · generation 0
Validation:
-
Current-head focused tagged tests passed: 15 top-level tests, 66 records including subtests, zero failures or skips, 99.133 seconds. Coverage includes endpoint ranges and uniqueness, allocation failures, cancellation, panic/Goexit cleanup, real bind collisions and the six-node gossip consumer.
-
Tagged logservice go vet, formatting and diff checks passed. Source remained unchanged.
-
Isolation preflight and a fresh repository-owned native build passed; native inputs and artifacts were fingerprinted.
-
Commands, source fingerprints, terminal receipts and logs are reconciled in /home/xupeng/.local/state/mo-bot/jobs/0dba3104b335e8c004c470a5/attempt-1/evidence-audit.json; review rationale is retained in review-record.json.
Limitations:
-
Validation used Linux and the short-mode consumer. Full repository/package runtime suites, the 24-node configuration and race tests were not rerun; focused coverage is sufficient for this test-only wiring change.
-
golangci-lint was unavailable; go vet and formatting checks passed.
-
The linked issue body and raw author CI logs were not supplied. Approval relies on source review and current-head local validation.
Merge Queue Status
This pull request spent 17 minutes 4 seconds in the queue, with no time running CI. ReasonPull request #30083 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
|
matrix-meow
added
size/XS
This branch had an error being deployed
4 participants
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #30047
What this PR does / why we need it:
Official main changes
allocatedPorts.allocateto accept a bounded listener range, but the gossip fixture still calls its obsolete one-argument signature. As a result, logservice tests cannot compile and both SCA and UT coverage fail, also blocking coverage aggregation. The same compiler error affects the SDK PRs #30054 and #30073–#30076, although their original diffs contain no logservice changes.Resolve
testPortRangebefore constructing the gossip cohort, return discovery errors to the existing fixture cleanup/recovery path, and pass both bounds to the shared allocator. This preserves the existing TCP/UDP probe and bounded allocation policy. Add a real configuration regression that checks all nine gossip/Raft/service endpoints from three nodes are unique and inside the permitted listener range.Validation
175a645a8599withgo vet -mod=readonly -tags matrixone_test ./pkg/logservice.go test -mod=readonly -tags matrixone_test -count=1 -run 'Test(TestPort|ProbeTestPort|GossipFixture)' ./pkg/logservice(95.107 seconds).gofmtandgit diff --checkpass.Local environment: Linux/amd64, Go 1.27.1, repository CGo headers/native libraries; socket tests ran with local network access. No test skips or weakened assertions were added. Remote CI on head
38578865e083ef8fa04962bfd62dbea914743102is complete: Linux/arm64 SCA, the full Ubuntu/x86 UT job, and CI Required all passed (run). Coverage and Compose BVT were skipped by normal path routing for this test-only PR; they are not counted as passes. The targeted local port regressions also passed under-racefor 20 executions (1.583s). This PR does not claim to fix the separate ISCP consumer timeout seen in one earlier coverage run.