Repository navigation
feat(sirius): introduce execution backend contract - #28973
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? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review @ 399e0ee
结论:approve。
Design gate:PASS。docs/design/sirius-embedded.md version 1 明确记录已批准的十 PR 计划,并定义本 PR 为 2/10 的 backend contract/Flight adapter 边界;native ABI、协议和后续 cutover 不属于本 PR。
实现与生命周期检查通过:
- Flight adapter 保留 Prepare 参数、失败清理和 fallback 分类;
- nil runtime 和 Prepare 失败不会产生 typed-nil execution;
- unsupported backend 在 lease、证书和 socket 初始化前 fail closed;
- Prepare/Run、cancel/join、partial failure、cleanup/reconciliation、consumer panic 路径未发现新增所有权或 liveness 问题;
- teardown 30 秒到 5 分钟以及 UT 外层 70 到 120 分钟的调整仍保留错误检查、TERM/KILL 和既有 per-package timeout,未发现 skip、retry、sleep 或断言弱化。
exact-head UT、BVT、SCA、build、coverage 和 CI Required 均通过。未发现 P1/P2 或 mandatory design blocker。
Merge Queue Status
This pull request spent 1 minute 15 seconds in the queue, with no time running CI. Waiting for any of
All conditions
ReasonPull request #28973 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. Tick the box to put this pull request back in the merge queue (same as
|
## What type of PR is this? - [x] API-change - [ ] BUG - [ ] Improvement - [ ] Documentation - [x] Feature - [x] Test and CI - [x] Code Refactoring ## Which issue(s) this PR fixes: Refs matrixorigin#28966 (migration PR 2 of 10). Numeric compatibility remains separately tracked by matrixorigin#28968. ## What this PR does / why we need it: Introduce the service-owned Sirius backend/execution contract so the compiler does not own concrete Flight types. The existing Flight adapter preserves its prepare, result delivery, fallback classification, reconciliation and cleanup behavior. No Flight protocol or native execution implementation changes here. - Record approved design version 1 and all ten PR boundaries in `docs/design/sirius-embedded.md`; no separate design-only PR. - Add `sirius.backend`, defaulting to `flight` during coexistence. `embedded` fails explicitly as unavailable in this build; unknown values fail closed. Both config validation and service startup reject unsupported selection before leases, certificates or sockets are initialized. - Make query cleanup depend on the narrow execution interface, retaining bounded cleanup after request cancellation and joined run/cleanup errors. - Preserve nil-backend validation and avoid a typed-nil execution on failed preparation. Keep malformed plans terminal rather than falling back. - Add fake-execution coverage for success, consumer failure, cancellation, cleanup failure and consumer panic; adapt existing real Flight/CN tests. - Correct two validation time budgets exposed by the first CI run: tenant fixture cleanup now has the scenario's bounded five-minute allowance, and the all-stage UT runner has a two-hour outer deadline. Per-package timeouts, assertions, stage selection and cancellation diagnostics are unchanged. This is the MO foundation only. It does not yet implement CGo, enable embedded queries, port matrixorigin#27599's MO-reader stream, change GPU scheduling, or remove Flight. PRs 3-8 provide those components; PRs 9-10 remain blocked on all-22 parity and numeric compatibility. The old matrixorigin#27599 branch is unchanged. ### Validation - Focused compiler/CN Sirius tests passed. - Full `pkg/sql/compile`, `pkg/sql/compile/sidecarflight`, `pkg/cnservice` tests passed normally and under `-race` through `mo-cgo-test`. - Finalized new tests (including consumer-panic cleanup) passed normally and under `-race`. - `make config` passed; no module metadata changed. - Focused golangci-lint with the repository's Go 1.26.4 toolchain: **0 issues**. - The original-head GitHub SCA, build, coverage and BVT jobs passed. Fresh exact-head CI is tracked separately below; no lint suppression was added. - GPU/SF10 tests are not claimed for this Go-only adapter foundation. The native synchronization is a separate Sirius PR. - Exact-head GitHub CI is pending. - Fresh post-main-merge validation with Go 1.26.4 and the existing GCC 15 CPU toolchain: tenant regression normal **11.708s**, race **19.549s**; all three compiler/Flight/CN owning packages passed normally and under race. - The UT-runner `optools` package passed (**6.175s**); incremental lint for compiler, CN and issue-test packages reported **0 issues**. Test selection, SQL assertions and cleanup error checks are unchanged. ### Review scope R0: design and delivery map. R2: backend/adapter/config consumer boundary. Lifecycle review follows admitted lease -> Prepare/execution -> Run -> Cleanup/retained reconciliation -> backend Close. The adapter adds no workers, queues or retry state. Existing Flight cancellation/reconciliation tests remain in the owning-package normal/race evidence. No new SQL behavior or wire representation is enabled, so no unrelated BVT/SF10 run is substituted for the focused real CN/Flight consumer tests. | Lifecycle check | Result for this PR | | --- | --- | | Q1: ownership | Prepare passes the same release callback to Flight; execution/reconciliation retain their existing cleanup owner. Failed preparation returns a genuinely nil interface. | | Q2: waits | The adapter adds no locks or waits. Cleanup still uses an independently bounded, uncanceled context and Flight's cancel/join acknowledgement. | | Q3: growth | One adapter per CN; no new queue, worker, retry collection or batch retention. | ### CI failure diagnosis and closure The first run's complete UT diagnostics identified two separate cutoffs: - `TestTenantCatalogRegressions` passed its scenario assertions, then its `DROP ACCOUNT` fixture cleanup exceeded 30 seconds while catalog DDL was still progressing. Cleanup now uses the same bounded five-minute allowance as the scenario; SQL assertions and cleanup error checks remain intact. - The overall runner received SIGTERM at its 70-minute outer deadline after light, exclusive-issues and embedded stages consumed most of that allowance. Heavy/engine tests were progressing and plan testing had not completed. The outer budget is now 120 minutes, with the existing per-package timeout and TERM/KILL diagnostic handling unchanged. No parallelism increase or test skip. CI evidence: [original failed UT job](https://github.com/matrixorigin/matrixone/actions/runs/35058948935/job/104675005101). Current-main integration includes `125d943786ecdf59be842ce381b9cc22d6a5c8bc`. Delivery head: `399e0ee601`. This foundation is independently reviewable; it does not wait for migration stages 3–8 to implement the native backend. --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
## What type of PR is this? - [x] API-change - [ ] BUG - [ ] Improvement - [ ] Documentation - [x] Feature - [x] Test and CI - [x] Code Refactoring ## Which issue(s) this PR fixes: Refs matrixorigin#28966 (migration PR 2 of 10). Numeric compatibility remains separately tracked by matrixorigin#28968. ## What this PR does / why we need it: Introduce the service-owned Sirius backend/execution contract so the compiler does not own concrete Flight types. The existing Flight adapter preserves its prepare, result delivery, fallback classification, reconciliation and cleanup behavior. No Flight protocol or native execution implementation changes here. - Record approved design version 1 and all ten PR boundaries in `docs/design/sirius-embedded.md`; no separate design-only PR. - Add `sirius.backend`, defaulting to `flight` during coexistence. `embedded` fails explicitly as unavailable in this build; unknown values fail closed. Both config validation and service startup reject unsupported selection before leases, certificates or sockets are initialized. - Make query cleanup depend on the narrow execution interface, retaining bounded cleanup after request cancellation and joined run/cleanup errors. - Preserve nil-backend validation and avoid a typed-nil execution on failed preparation. Keep malformed plans terminal rather than falling back. - Add fake-execution coverage for success, consumer failure, cancellation, cleanup failure and consumer panic; adapt existing real Flight/CN tests. - Correct two validation time budgets exposed by the first CI run: tenant fixture cleanup now has the scenario's bounded five-minute allowance, and the all-stage UT runner has a two-hour outer deadline. Per-package timeouts, assertions, stage selection and cancellation diagnostics are unchanged. This is the MO foundation only. It does not yet implement CGo, enable embedded queries, port matrixorigin#27599's MO-reader stream, change GPU scheduling, or remove Flight. PRs 3-8 provide those components; PRs 9-10 remain blocked on all-22 parity and numeric compatibility. The old matrixorigin#27599 branch is unchanged. ### Validation - Focused compiler/CN Sirius tests passed. - Full `pkg/sql/compile`, `pkg/sql/compile/sidecarflight`, `pkg/cnservice` tests passed normally and under `-race` through `mo-cgo-test`. - Finalized new tests (including consumer-panic cleanup) passed normally and under `-race`. - `make config` passed; no module metadata changed. - Focused golangci-lint with the repository's Go 1.26.4 toolchain: **0 issues**. - The original-head GitHub SCA, build, coverage and BVT jobs passed. Fresh exact-head CI is tracked separately below; no lint suppression was added. - GPU/SF10 tests are not claimed for this Go-only adapter foundation. The native synchronization is a separate Sirius PR. - Exact-head GitHub CI is pending. - Fresh post-main-merge validation with Go 1.26.4 and the existing GCC 15 CPU toolchain: tenant regression normal **11.708s**, race **19.549s**; all three compiler/Flight/CN owning packages passed normally and under race. - The UT-runner `optools` package passed (**6.175s**); incremental lint for compiler, CN and issue-test packages reported **0 issues**. Test selection, SQL assertions and cleanup error checks are unchanged. ### Review scope R0: design and delivery map. R2: backend/adapter/config consumer boundary. Lifecycle review follows admitted lease -> Prepare/execution -> Run -> Cleanup/retained reconciliation -> backend Close. The adapter adds no workers, queues or retry state. Existing Flight cancellation/reconciliation tests remain in the owning-package normal/race evidence. No new SQL behavior or wire representation is enabled, so no unrelated BVT/SF10 run is substituted for the focused real CN/Flight consumer tests. | Lifecycle check | Result for this PR | | --- | --- | | Q1: ownership | Prepare passes the same release callback to Flight; execution/reconciliation retain their existing cleanup owner. Failed preparation returns a genuinely nil interface. | | Q2: waits | The adapter adds no locks or waits. Cleanup still uses an independently bounded, uncanceled context and Flight's cancel/join acknowledgement. | | Q3: growth | One adapter per CN; no new queue, worker, retry collection or batch retention. | ### CI failure diagnosis and closure The first run's complete UT diagnostics identified two separate cutoffs: - `TestTenantCatalogRegressions` passed its scenario assertions, then its `DROP ACCOUNT` fixture cleanup exceeded 30 seconds while catalog DDL was still progressing. Cleanup now uses the same bounded five-minute allowance as the scenario; SQL assertions and cleanup error checks remain intact. - The overall runner received SIGTERM at its 70-minute outer deadline after light, exclusive-issues and embedded stages consumed most of that allowance. Heavy/engine tests were progressing and plan testing had not completed. The outer budget is now 120 minutes, with the existing per-package timeout and TERM/KILL diagnostic handling unchanged. No parallelism increase or test skip. CI evidence: [original failed UT job](https://github.com/matrixorigin/matrixone/actions/runs/35058948935/job/104675005101). Current-main integration includes `125d943786ecdf59be842ce381b9cc22d6a5c8bc`. Delivery head: `399e0ee601`. This foundation is independently reviewable; it does not wait for migration stages 3–8 to implement the native backend. --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
What type of PR is this?
Which issue(s) this PR fixes:
Refs #28966 (migration PR 2 of 10). Numeric compatibility remains separately tracked by #28968.
What this PR does / why we need it:
Introduce the service-owned Sirius backend/execution contract so the compiler
does not own concrete Flight types. The existing Flight adapter preserves its
prepare, result delivery, fallback classification, reconciliation and cleanup
behavior. No Flight protocol or native execution implementation changes here.
docs/design/sirius-embedded.md; no separate design-only PR.sirius.backend, defaulting toflightduring coexistence.embeddedfails explicitly as unavailable in this build; unknown values fail closed.
Both config validation and service startup reject unsupported selection
before leases, certificates or sockets are initialized.
bounded cleanup after request cancellation and joined run/cleanup errors.
preparation. Keep malformed plans terminal rather than falling back.
cleanup failure and consumer panic; adapt existing real Flight/CN tests.
fixture cleanup now has the scenario's bounded five-minute allowance, and
the all-stage UT runner has a two-hour outer deadline. Per-package timeouts,
assertions, stage selection and cancellation diagnostics are unchanged.
This is the MO foundation only. It does not yet implement CGo, enable embedded
queries, port #27599's MO-reader stream, change GPU scheduling, or remove Flight.
PRs 3-8 provide those components; PRs 9-10 remain blocked on all-22 parity and
numeric compatibility. The old #27599 branch is unchanged.
Validation
pkg/sql/compile,pkg/sql/compile/sidecarflight,pkg/cnservicetests passed normally and under-racethroughmo-cgo-test.-race.make configpassed; no module metadata changed.exact-head CI is tracked separately below; no lint suppression was added.
native synchronization is a separate Sirius PR.
toolchain: tenant regression normal 11.708s, race 19.549s; all three
compiler/Flight/CN owning packages passed normally and under race.
optoolspackage passed (6.175s); incremental lint forcompiler, CN and issue-test packages reported 0 issues. Test selection,
SQL assertions and cleanup error checks are unchanged.
Review scope
R0: design and delivery map. R2: backend/adapter/config consumer boundary.
Lifecycle review follows admitted lease -> Prepare/execution -> Run ->
Cleanup/retained reconciliation -> backend Close. The adapter adds no workers,
queues or retry state. Existing Flight cancellation/reconciliation tests remain
in the owning-package normal/race evidence. No new SQL behavior or wire
representation is enabled, so no unrelated BVT/SF10 run is substituted for
the focused real CN/Flight consumer tests.
CI failure diagnosis and closure
The first run's complete UT diagnostics identified two separate cutoffs:
TestTenantCatalogRegressionspassed its scenario assertions, then itsDROP ACCOUNTfixture cleanup exceeded 30 seconds while catalog DDL was stillprogressing. Cleanup now uses the same bounded five-minute allowance as the
scenario; SQL assertions and cleanup error checks remain intact.
light, exclusive-issues and embedded stages consumed most of that allowance.
Heavy/engine tests were progressing and plan testing had not completed. The
outer budget is now 120 minutes, with the existing per-package timeout and
TERM/KILL diagnostic handling unchanged. No parallelism increase or test skip.
CI evidence: original failed UT job.
Current-main integration includes
125d943786ecdf59be842ce381b9cc22d6a5c8bc.Delivery head:
399e0ee601.This foundation is independently reviewable; it does not wait for migration
stages 3–8 to implement the native backend.