Repository navigation
Conversation
linrrzqqq
requested review from
924060929,
CalvinKirs,
englefly,
morrySnow and
starocean999
as code owners
October 19, 2026 03:34
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Collaborator
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed all 58 changed paths at b94dd43a8bdd9c7b85306bb23e88a4c807241af0. Three P2 defects are reported inline: dropped generations can be recreated by late queries (F1), native import can resolve a supplied UDF entry point to a preloaded server module (F2), and temporary backend outages permanently lose database cleanup notifications (F3). No P0/P1 finding was identified. There were no existing inline threads to duplicate or carry as blockers, and no additional user-provided focus points.
Required critical checkpoint conclusions:
- Goal and proof: Per-function capacity and idle suspension are wired through FE metadata and all three BE function types. Retirement and import compatibility still have the defects above. Added unit/regression sources cover the intended behavior; their execution was not verified in this review.
- Scope and reuse: Changes are focused on Python process/package ownership, settings, and cleanup. Shared metadata copies, Thrift conversion, pool management, and client leases provide the common mechanisms.
- Concurrency and locks: Foreground opens, detached initialization/coordinator/repair workers, the monitor, and cleanup tasks were traced. Map -> pool -> process-registry locking has no new reverse acquisition found. Fork, process termination, and cleanup submission run outside metadata/pool locks where required. F1 concerns acquisition after retirement rather than an ordinary lock inversion.
- Lifecycle and static initialization: Leases protect initialization and active clients; process/package ownership and weak RSS registry avoid cycles. No new cross-translation-unit static initialization dependency was found. F1 and F3 leave dropped generations alive; F1 can also invalidate a later package owner.
- Configuration:
pool_sizedefaults to 3 and must be positive; any negativeauto_suspenddisables suspension, with default -1. These are persisted CREATE properties, not mutable BE settings. Nonnegative timeouts are checked by the 30-second monitor. Removal ofmax_python_process_numis reflected in all changed pipeline configurations. - Compatibility: Missing Thrift fields use explicit BE defaults 3/-1, and legacy JSON uses initialized metadata defaults. Return types, nullable behavior, and aggregate intermediate representations are unchanged. F2 is a demonstrated module-name compatibility regression. No release binary ABI audit was performed.
- Thrift/protobuf: Complete
TFunctiondefinitions were checked on HEAD and origin/branch-3.1, branch-4.0, branch-4.1, and branch-4.2. IDs 20/21 are unused on those branches; both additions are optional. No protobuf change. - Parallel paths: Scalar UDF, aggregate UDAF, table UDTF, outer UDTF, global/database functions, expression copies, and catalog reconstruction retain the settings. Normal and outer UDTFs receive distinct IDs. All three BE open paths pin module packages and acquire clients.
- Conditional/error handling: Parsing, nonpositive capacity, shutdown, stopped-pool, and empty-cleanup branches have concrete failure/lifecycle reasons. Existing client errors release leases after cleanup. No additional ignored Status or exception-boundary regression was found. Module binding publishes its root and identity under one lock.
- Test coverage: New source covers defaults/explicit properties, integer errors/overflow, Java rejection, copies/JSON/Thrift, database recovery/erase modes, independent pools, active RSS, shutdown, and idle lease behavior. The late-retirement, preloaded-module-name, and unavailable-backend cases behind the findings are missing.
- Expected results: Scalar 11/12/13, UDAF sum 6, outer-UDTF NULL, and native-cache value 1107/12 rows match their definitions statically. Both new zip members match checked-in helper sources and old archive entries are preserved. Automatic generation of
.outfiles was not independently established. - Observability: Creation logs identify function/runtime/capacity; cleanup logs identify backend/function; RSS remains visible after DROP. The memory threshold only warns, so it cannot reclaim pools leaked by F1/F3. No additional demonstrated logging/metric defect was found.
- Persistence and failover: Fields have Gson annotations and survive shared copy/image/add-function replay paths. Recoverable database drops retain function generations. Cleanup runs after metadata removal; its notification has no retained retry/reconciliation, as discussed in F3.
- Writes and atomicity: No table-data, transaction-visibility, visible-version, rowset, or delete-bitmap path changes. Existing metadata logging and table-lock ordering remain. The reported failures concern query/module execution and resource retirement.
- FE-to-BE variables: The central scalar/aggregate converters, all Nereids Python reconstruction paths, and all three BE extraction sites propagate the new settings. Non-Python Thrift functions leave them unset; no separate sending path was found to omit them.
- Performance and memory: Pool capacity now scales with used function generations; the weak registry accounts for dropped-but-active processes. No additional verified hot-path regression was found. F1/F3 make this resource growth permanent for dropped generations.
- Additional issues: A final upstream/downstream and special-case sweep found no other substantiated defect beyond the three inline comments.
Validation and limits: no builds, Doris unit/regression tests, cluster startup, or source modifications were performed. An isolated review script extracted the actual base/head Python loader definitions: a supplied types.py entry point succeeds on the base and fails on this head. Distributed race schedules and backend outages were established from control flow, not exercised. Regression idle sleeps are shorter than the 30-second monitor interval, and the concurrent-import suite warms the module before concurrent calls; those suites do not independently prove restart or concurrent first import. These execution limits do not leave an undisposed review candidate or an uncovered changed path.
Review converged after 3 round(s).
linrrzqqq
requested review from
deardeng,
gavinchou and
mymeiyi
as code owners
October 19, 2026 17:50
Collaborator
Author
|
/review |
Collaborator
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Static review of all 68 changed paths at 77973138307951d290ed94cb8cc86b4a30424d46. Two new P2 findings concern cleanup delivery for ordinary/global DROP FUNCTION and permanent database erasure across FE master failover. No new P0/P1 finding was verified.
Checkpoint conclusions:
- Goal and proof: fixed per-function pools, persistent imports, package pins and idle policy are implemented across scalar UDF, UDAF and UDTF. Added unit/regression tests support the main flows; cleanup remains incomplete in the two new findings and the existing retirement/import threads.
- Scope and reuse: changes are focused on this lifecycle refactor. The ordinary DROP path still needs the queued cleanup mechanism used by database erasure.
- Concurrency: query, agent, monitor and detached fork/repair entrants were traced. Map-to-pool-to-registry locking is consistent; process shutdown and monitor join occur outside map/pool locks. FE cleanup submission occurs after metadata locks are released. No additional lock cycle was established.
- Lifecycle and initialization: leases protect client initialization through stream/state cleanup; package handles pin lazy imports; weak registry entries retain visibility for RSS without owning processes. No new ownership cycle or cross-translation-unit initialization dependency found. Dropped-generation retirement remains incomplete in the existing thread.
- Configuration:
pool_sizedefaults to 3 and rejects nonpositive values;auto_suspenddefaults to -1, with negative values disabling suspension and nonnegative values observed by the monitor. These are persisted CREATE properties rather than mutable BE settings. Removal ofmax_python_process_numis reflected in the changed pipeline configurations. - Compatibility: old JSON defaults, Function copies, expression copies and optional-field BE defaults were inspected. No new persisted storage-format incompatibility found; older BEs ignore the new optional tuning fields.
- Thrift/protobuf: complete
TFunctiondefinitions were compared with branch-4.2, branch-4.1, branch-4.0 and branch-3.1 snapshots. IDs 20/21 are unused there and optional here; existing IDs retain their meanings. No protobuf change. - Parallel paths: scalar/aggregate/table normal and outer functions, local/global registration and DROP, inline/module loading, and common cloud/shared-nothing execution paths were traced. The ordinary/global cleanup omission is one new finding.
- Conditions and errors: startup/STOPPED/idle checks and cleanup error acknowledgements have concrete scenarios; failed task acknowledgements remain queued and success removes only the acknowledged generation. No additional silent-error regression verified.
- Test coverage: inspected BE lifecycle/cache/ack tests, FE validation/copy/serialization/recycle/retry tests, and Python regression suites. Ordinary/global DROP outage and pending-cleanup master failover are missing failure cases. Existing late-retirement and preloaded-module collisions also lack coverage.
- Expected results: all seven new output files match the inspected query/package semantics and ordering. The changed ZIP adds only the two native-cache helpers, byte-identical to their loose sources; prior entries are unchanged. Output-generation provenance was not verified.
- Observability: pool initialization, process errors/exits and cleanup logs include useful runtime/function/backend context; existing task proc output exposes queued and failed cleanups, and the process registry retains RSS accounting for active dropped clients. No separate observability defect verified.
- Persistence and failover: new properties flow through existing Function image/journal/replay mechanisms. Pending cleanup after permanent database erasure is only process-local, producing the second new finding.
- Data writes and crashes: no database data-write, visible-version, delete-bitmap or transaction protocol changes. Process failure/reaping and asynchronous cache deletion paths were inspected; retained pool resources are the identified crash/failover consequence.
- FE-BE propagation: both thrift converters, all three Python expression families and their copy paths, and scalar/UDAF/UDTF BE consumers carry both settings with matching defaults; shared expression serialization covers constant/point uses.
- Performance and memory: fixed pools and native imports remove shared-runtime interference and repeated import eviction. Notification work scales with backends times functions; RSS accounting includes active retained processes. The verified cleanup gaps can accumulate persistent pools. No additional concrete hot-path regression found.
- Other checks: touched code preserves function type/nullability and aggregate intermediate semantics; changed headers introduce no PCH, unity-skip or explicit-instantiation-family change requiring a separate finding.
Existing context: late-query retirement still applies before the first pool and after STOPPED reclamation; preloaded module collision also remains. Both are existing P2 findings and were not reposted. The original database cleanup during a backend outage is fixed for same-master recovery by queueing, resend and acknowledgement; the new failover finding concerns a separate persistence boundary. No existing P0/P1 comment applies, so the blocking-ID list is empty.
No additional user focus was supplied. Static analysis only: no builds, tests, cluster execution or source changes. Runtime behavior, compile/link artifacts and generated-output provenance were not verified. No unresolved static candidate or changed-path coverage gap remains after the final sweep. Review converged in two rounds.
Review converged after 2 round(s).
Contributor
TPC-H: Total hot run time: 28946 ms |
Contributor
TPC-DS: Total hot run time: 150803 ms |
Contributor
ClickBench: Total hot run time: 24.25 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Collaborator
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of PR #69255 at 241b1f1a712091824e5c05a7a8fdfd3ecbc3fc2c: no new distinct verified inline findings. This does not resolve the existing concerns below.
Existing-thread dispositions:
- Late work recreating dropped generations remains applicable. STOPPED pools reject new clients while retained, but DROP before pool creation and acquisition after monitor removal can still recreate the retired id. Kept in its existing thread.
- Package entry points shadowed by preloaded modules remains applicable. Native imports still consult preloaded
sys.modules; the author's avoidance reply is known context. Kept in its existing thread. - Cleanup lost across master failover remains applicable. Pending tasks live only in the active FE's static queue, and database-erasure replay does not reconstruct them. Kept in its existing thread.
- Database cleanup retry after backend recovery and local/global DROP retry are addressed on this head: all these paths register id-scoped tasks through the shared helper, and successful acknowledgements remove them.
Critical checkpoint conclusions:
- Goal and evidence: per-function pools, capacity and idle timeout are wired through FE metadata, BE acquisition and the Python server. New unit/regression cases exercise these behaviors; the three existing limitations above prevent claiming complete lifecycle correctness.
- Scope and reuse: production changes remain focused on Python scheduling, ownership, metadata and cleanup. The shared cleanup helper also consistently serves local/global DROP; most regression edits opt into idle reclamation.
- Concurrency and locks: foreground clients, detached initialization/repair, the monitor and cleanup workers were traced. Map-to-pool locking and lease acquisition before initialization protect idle reclamation; process termination runs outside map/pool locks. No additional deadlock or active-client reclamation defect was verified.
- Lifecycle and static initialization: leases outlive stream cleanup, package handles pin files, detached workers retain pool/registry owners, and STOPPED blocks late publication. No new cross-translation-unit static initialization dependency was found. Retirement and failover gaps remain in the existing threads.
- Configuration: the global process-count setting is removed;
pool_sizeandauto_suspendare persisted per-function properties. New generations acquire the configured values, and the 30-second monitor applies nonnegative idle timeouts. Negative timeouts retain pools; runtime ALTER support is not introduced. - Compatibility: absent property defaults, function copies and serialization paths were checked. New BE readers default missing fields; old readers can ignore the optional additions. An old BE does not implement the new cleanup acknowledgement, so queued tasks can remain until it is upgraded; id-scoped cleanup is still executed.
- Shared definitions:
TFunctionwas compared with livebranch-3.1,branch-4.0,branch-4.1andbranch-4.2. IDs 20/21 are unused there, remain optional, and do not change existing field meanings. - Parallel paths: scalar/UDAF/UDTF, generated outer functions, expression copies, both thrift converters, local/global DROP, force DROP DB and all three recycler erasure paths were checked. No additional propagation omission was verified.
- Conditional checks: positive pool-size validation, absent-field fallbacks, idle eligibility, stopped publication guards and acknowledgement conditions have concrete triggering cases. No additional incorrect branch was verified.
- Coverage: added BE/FE unit tests cover pool isolation, leases, reclamation, invalid properties, metadata copies, package lifetime and retry/acknowledgement. Regression cases cover SQL results, native/lazy imports and backend outage recovery. They do not prove master-failover durability or permanent retirement, and 2.5-second SQL sleeps alone do not guarantee a 30-second monitor pass; dedicated BE tests exercise termination.
- Results and artifacts: expected values were checked against suite inputs; the updated zip adds exactly the two new native-cache scripts, matching their source bytes while preserving original entries. Execution and result-file generation were not independently reproduced.
- Observability: function-id initialization/cleanup logs, process errors and aggregate RSS tracking remain available, including accepted initialization/repair processes after DROP. No separate observability defect was verified.
- Persistence and failover: Gson fields/defaults and journal/replay paths were inspected. Cleanup intent durability is still the existing failover finding; same-master retries now work through the agent queue.
- Data mutation and crashes: table-write, visibility/version and delete-bitmap protocols are unchanged. Metadata deletion and process cleanup are asynchronous; crash-related resource retention is covered by the existing durability concern.
- FE-to-BE variables: both properties pass through scalar, aggregate, table/outer, catalog-copy, Nereids-copy and thrift paths; all three BE consumers supply compatible defaults.
- Performance: pool size bounds each function's processes, selection favors the least-shared live process, and import caching avoids repeated module loading. Per-function resource growth and monitor scans follow the intended ownership model; no additional concrete hot-path regression was verified.
- Other concerns and focus: all 144 changed paths were covered. No additional user focus was supplied. No new unique verified issue remains outside the existing threads.
Validation was static only: no builds, tests, cluster execution or source modifications. Convergence concerns the completed review and deduplicated finding set; it does not mean the existing P2 issues are fixed.
Review converged after 1 round(s).
Collaborator
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28566 ms |
Contributor
TPC-DS: Total hot run time: 150511 ms |
Contributor
ClickBench: Total hot run time: 25.65 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
linrrzqqq
force-pushed
the
pyudf-process-pool
branch
from
October 21, 2026 07:43
241b1f1 to
e83bfc0
Compare
Collaborator
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
ClickBench: Total hot run time: 24.4 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Collaborator
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed all 150 changed paths at e83bfc03ce541b8c30e7842a91a8a81b2e3bf2a1 using static analysis, including the scalar/UDAF/UDTF execution paths, FE metadata and cleanup, release-branch protocol definitions, tests, expected results, configuration changes, and packaged Python helpers. No additional distinct finding was substantiated. Three existing P2 concerns still apply and should be addressed; they are not reposted inline.
Existing review context:
- Late work can recreate a dropped generation: STOPPED rejects new clients while its pool remains mapped, but an absent pool or later monitor removal leaves no retirement fence. UDAF open can retain metadata/package ownership before acquiring its first client lease. The related same-path package deletion concern remains covered by this thread.
- Entry-point imports can resolve a preloaded server module: process isolation and adding the package directory do not override
sys.modules; the suppliedtypes.pycase still applies. The author's reply was considered. - Database cleanup intent is lost across master failover: queue registration improves retry while the same FE remains master, but the static task queue is not persisted or reconstructed by database-erasure replay.
- The database backend-outage retry concern and ordinary/global DROP retry concern are addressed on this head: both routes register generation-specific tasks, report handling resends missing tasks, and successful backend acknowledgements remove them.
Critical checkpoint conclusions:
| Checkpoint | Conclusion |
|---|---|
| Goal and evidence | Independent function-ID pools and the two properties are implemented throughout the three function types. Unit/regression coverage supports the intended behavior at source level; the existing retirement, import, and failover concerns qualify completion of that behavior. |
| Scope and clarity | Source changes form a cohesive lifecycle refactor; most regression edits only set pool capacity and idle suspension. Shared lease, registry, and cleanup helpers have distinct responsibilities. |
| Concurrency and lock cost | Traced query, cleanup, monitor, detached initialization/repair, and Flight threads. Manager map precedes pool locking; registry snapshots do not invert that order. Fork, RPC/client construction, RSS reads, and retired-process shutdown occur outside pool/map locks. No additional lock inversion was found. |
| Ownership and static initialization | Leases cover client initialization through stream cleanup, including error exits; pool/process handles pin package files. Detached workers retain pool/registry ownership and reject STOPPED publication. Weak registry entries account for dropped active processes without an ownership cycle. No new cross-translation-unit static dependency was found. The remaining retirement hole is the existing thread above. |
| Configuration | Removed max_python_process_num consistently. Function properties persist defaults of 3/-1; positive pool-size validation and negative/zero/positive suspension behavior are consistent across paths. No new mutable BE configuration was added. |
| Compatibility | New BE paths supply defaults for absent fields; FE constructors, copies, Gson metadata, expression copies, SQL display, and conversion routes preserve both properties. Older BEs perform cleanup but lack the new acknowledgement: task bookkeeping remains pending until BE upgrade/restart permits report-driven retry to the new callback. No distinct execution regression was substantiated from that rollout limit. |
| Thrift/protobuf | Read complete TFunction definitions on HEAD and current remote-matching branches 3.1, 4.0, 4.1, and 4.2. IDs 20/21 are unused there, optional here, and do not reuse another meaning. No protobuf change. |
| Parallel paths | Checked scalar/UDAF/UDTF and outer variants, local/global function drops, force/expired/same-name/instant database erasure, recoverable database retention, and cloud/shared-nothing backend enumeration. |
| Conditions and error handling | New lifecycle branches explain their concrete triggering cases. Cleanup status/acknowledgement and lease-release paths were traced; existing best-effort Java cleanup behavior is retained. No additional unhandled failure path was substantiated. |
| Test breadth and negative cases | Inspected invalid property values, metadata copies/serialization, retry/acknowledgement failures, pool isolation, initialization/repair, leases, idle duration, RSS, DROP, and package tests. The retirement tests do not attempt acquisition after reclamation; the native-cache concurrency test warms its module first. These qualify test evidence for the already-known concerns. |
| Changed expected results | All seven new result files were checked against their query inputs and inline/packaged implementations. ZIP changes add the matching two helpers without changing existing entries. Test execution and generated-file provenance were not verified. |
| Observability | Pool creation/cleanup includes generation identifiers; existing runtime/PID failure logs, task status/reporting, and RSS warnings cover the changed lifecycle. No separate new metrics requirement was established. |
| Persistence and failover | Both properties use serialized metadata fields. Recoverable databases retain their function generations. Permanent erasure cleanup still has the existing journal/replay durability gap described above. No data transaction protocol is changed. |
| Data writes and crash behavior | No rowset/version/delete-bitmap or transaction atomicity change. Process/package cleanup crash and failover behavior was reviewed; remaining issues are covered by the existing threads. |
| FE-to-BE transmission | Both converter overloads and all three expression/catalog/BE entry paths preserve pool size and suspension, including copied expressions and UDTF outer functions. |
| Performance | Fixed per-function capacity, least-shared live-process selection, retained native imports, and weak child accounting match the intended design. No additional demonstrated performance defect was found. |
| Other module requirements | No new hub/PCH include, gate-table edit, explicit-instantiation family, source-including test, or suspect libc/TLS/link dependency was introduced. Final GLIBC 2.17 ELF compatibility was not validated. |
There was no additional user-provided focus. Review convergence means the review investigation is complete; it does not mean the existing P2 bugs are fixed. No applicable existing P0/P1 finding was identified. Builds, tests, a live cluster, final binary ABI checks, and regression-output generation were not run under this static-only contract.
Review converged after 1 round(s).
Contributor
|
I think these two parameters need tighter resource bounds and a different default:
|
linrrzqqq
added a commit
to linrrzqqq/doris
that referenced
this pull request
Oct 22, 2026
### What problem does this PR solve? Related PR: apache#69255 Problem Summary: Per-function Python pools could allocate and start as many processes as requested, and their default idle policy retained processes indefinitely. Cap each pool at the executing backend's CPU core count before allocating slots or starting processes, while retaining positive pool_size validation. Set auto_suspend to 14400 seconds consistently across function creation, catalog metadata, Thrift, and backend defaults. Require explicit -1 to disable idle reclamation and reject smaller values. ### Release note Python function pools are capped by each backend's CPU core count and are reclaimed after four hours without active clients by default. The next call recreates the pool. Explicit auto_suspend=-1 disables idle reclamation. ### Check List (For Author) - Test: Full build; 38 FE unit tests; 29 BE unit tests on both host and one-core configurations; four targeted regression suites; all 19 Python UDTF suites; verified pool_size=10000 starts exactly one Python server on a one-core BE; clang-format 16 and clang-tidy checks. - Behavior changed: Yes, cap effective pool_size per backend and default auto_suspend to four hours; reject auto_suspend values below -1. - Does this need documentation: Yes, English and Chinese development-version descriptions are updated in the separate doris-website working directory.
linrrzqqq
force-pushed
the
pyudf-process-pool
branch
from
October 22, 2026 13:08
e83bfc0 to
1118af0
Compare
Collaborator
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of head 1118af03752ccfb9b7d5cc4d2746786cbd1b66b0, using the supplied aggregate diff and all 151 changed paths. Main review plus three independent reviewers completed one round; all three returned NO_NEW_VALUABLE_FINDINGS. Review converged. No additional verified inline finding beyond existing threads; no applicable existing P0/P1 finding.
Existing review dispositions:
- Late queries recreating dropped generations still applies when DROP sees no pool or after the STOPPED pool is reclaimed. Retaining STOPPED pools while clients exist protects that narrower interval. Not reposted.
- Module entry point resolving to an already imported module still applies; the author reply was considered. Native module caching does not make sys.path override an existing sys.modules entry. Not reposted.
- Cleanup intent lost on master failover still applies: queued generation IDs are not reconstructed during metadata replay. Not reposted.
- Database cleanup retry after backend outage and ordinary/global DROP retry are fixed here through the shared queued helper, resend path, completion acknowledgement and running-signature removal.
Current defaults are pool_size=3, CPU-capped on BE, and auto_suspend=14400 seconds. The PR description and historical thread wording that call -1 the default are stale; explicit -1 still disables suspension. This also limits the indefinite-process-retention examples in existing threads to explicit -1; missing cleanup can still retain package-cache entries.
Critical checkpoint conclusions:
- Goal and proof: per-function isolation, size/idle properties, package ownership and cleanup retries are implemented across scalar UDF, UDAF and UDTF. Added unit/regression cases support these paths by inspection. Known lifecycle gaps remain as linked above.
- Scope and reuse: production changes stay focused on Python scheduling/lifecycle; the shared cleanup helper and existing metadata converters avoid separate implementations. Regression property updates consistently use small reclaimable pools.
- Concurrency: query/group creation, detached initialization/repair, monitor and cleanup worker interactions were traced. Map-before-pool lock order and shared repair guards are consistent; lease acquisition protects client initialization against reclamation. Fork and process shutdown are outside the map lock. FE submits cleanup after metadata locks release.
- Lifecycle/static initialization: cache handles reach all three function forms, pools and accepted processes; streams/state/process references are cleaned before lease release. Detached workers retain pool/registry ownership and reject STOPPED publication; the weak process registry avoids ownership cycles and retains accounting visibility. No new cross-translation-unit initialization dependency found. Retired-ID protection remains the existing concern.
- Configuration: the global max_python_process_num setting is removed, including pipeline configurations. New function properties are immutable DDL metadata, validated for positive i32 capacity and i64 timeout >= -1; no dynamic ALTER behavior is introduced.
- Compatibility: old JSON and absent wire fields receive consistent defaults; copies and serialization preserve explicit values. Older peers ignore optional fields. Runtime mixed-version execution was not performed.
- Thrift/protobuf IDs: complete TFunction definitions on HEAD and origin/branch-3.1, branch-4.0, branch-4.1, branch-4.2 and master were compared. New IDs 20/21 are unused on those release branches, optional, and existing IDs retain their meanings. No protobuf change.
- Parallel paths: scalar/aggregate/table, inline/module, normal/outer UDTF, volatile/distinct expression copies, local/global DROP, force/expiry/same-name/instant database erase, recoverable database retention, constant folding and point serialization were traced. Cloud backend enumeration retains all compute-group coverage.
- Conditional/error handling: range checks, empty cleanup maps, STOPPED states and active-client/idle gates correspond to concrete invalid-input or lifecycle cases. Initialization/client failures release leases; failed cleanup acknowledgement retains retry intent. No additional distinct error-path defect established.
- Test coverage: inspected unit tests cover resource validation/defaults/copies, pool isolation, active leases, initialization failures, reclamation, memory accounting, lost acknowledgement and backend recovery. Regression additions cover idle settings/PID replacement, normal/outer forms, lazy/concurrent imports and cleanup retry. Failover/retired-ID gaps remain the existing findings.
- Expected results: all seven new output files were matched to their queries; deterministic arithmetic, null and aggregate results agree by inspection. The changed ZIP adds only the two matching native-cache helpers; its 22 existing members are byte-identical. Generated-result provenance was not independently established.
- Observability: function/backend IDs, existing task status/resend accounting, fork/repair failures and process RSS tracking cover the changed paths. No additional required metric omission established.
- Persistence/failover: new properties use existing Function image/EditLog serialization and replay. Pending cleanup intent remains in memory only, as the existing failover finding explains.
- Writes/atomicity/crashes: transaction visibility, rowsets and delete bitmaps are unchanged. Metadata locking/journaling and process failure transitions were inspected; no additional data-write or crash-consistency defect found.
- FE-BE variables: both new fields propagate through catalog constructors/copies, Nereids roundtrips and scalar/aggregate converters into all three BE consumers, with identical missing-field defaults.
- Performance: capacities are CPU-capped before allocation, imports reuse Python's module cache, and cleanup queues deduplicate generation IDs. Serial monitor repair can lengthen scans as function count grows; the repair mechanism preexists and no distinct new correctness failure or violated reclamation deadline was established.
- Other checks: unchanged null/Arrow semantics, header/include changes, template instantiations and new unit-test includes were inspected; no new static hygiene or glibc API dependency issue identified. Final missed-case sweep found no additional substantiated defect.
User focus: none was supplied. Coverage is complete for the supplied changed-path list; no unresolved new candidates or unreviewed paths remain. Validation is static only: no builds, tests, sanitizer runs, cluster execution or release-binary ABI inspection were performed. Convergence describes completion of this review, not resolution of the previously reported P2 issues.
Review converged after 1 round(s).
Collaborator
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28734 ms |
Contributor
TPC-DS: Total hot run time: 150865 ms |
Contributor
ClickBench: Total hot run time: 25.18 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Problem Summary: Refactor Python UDF process scheduling to maintain an independent process pool for each function id. Each function owns a fixed-size pool, simplifying allocation, package isolation and lifecycle management for Python UDF, UDAF and UDTF. As part of the refactoring, add two function properties: pool_size controls the maximum process count per function on each BE and defaults to 3; auto_suspend specifies the idle timeout in seconds and defaults to -1 to keep pools resident. The existing 30-second monitor terminates eligible idle pools and subsequent invocations recreate them. Active clients, including clients being initialized, prevent reclamation. The new properties are persisted in function metadata and rejected for Java functions. Adapt DROP FUNCTION and permanent database removal to release the corresponding Python pools. Existing clients retain their processes and package files until release, and those processes remain included in memory accounting. Prevent deleted function ids from recreating package-cache entries that share files with active clients. Keep the existing one-shot cleanup notification behavior. ### Release note Python functions use independent process pools. The pool_size and auto_suspend properties control pool capacity and idle reclamation. ### Check List (For Author) - Test: Full BE/FE build; 83 BE unit tests; 46 FE unit tests; clang-tidy and header hygiene; cluster startup and basic query verification. Python regression suites were exercised during implementation. - Behavior changed: Yes, Python process allocation and lifecycle management are scoped by function id, with configurable pool size and idle reclamation. - Does this need documentation: Yes, English and Chinese documentation changes are prepared separately in doris-website.
### What problem does this PR solve? Issue Number: N/A Problem Summary: Keep dropped function pools STOPPED in the manager while existing clients are alive, so a running UDAF cannot recreate the pool when it encounters another GROUP BY key. Reclaim stopped pools in the 30-second health check after the last client closes, regardless of auto_suspend. Document the distinction between DROP cleanup and idle suspension, and cover UDF/UDAF/UDTF client rejection and same-name function recreation. ### Release note None ### Check List (For Author) - Test: BE/FE build, 83 BE unit tests, 20 regression suites (including all 18 pythonudtf_p0 suites), clang-tidy, formatting and header hygiene passed for the lifecycle changes. The final equivalent guard consolidation was checked for boolean equivalence and formatting/header hygiene without rerunning the full suite. BE UT compilation used temporary adaptations to three unrelated baseline Variant test files; those adaptations were restored. - Behavior changed: Yes, DROP retains a STOPPED pool until the monitor observes no active clients and reclaims it. - Does this need documentation: No
Queue local and global DROP FUNCTION cleanup for retry after backend recovery. Set auto_suspend=0 in ordinary Python regression cases to release idle processes. Validation: full build, 17 FE unit tests, and 86 Python regression suites passed.
### What problem does this PR solve? Related PR: apache#69255 Problem Summary: Per-function Python pools could allocate and start as many processes as requested, and their default idle policy retained processes indefinitely. Cap each pool at the executing backend's CPU core count before allocating slots or starting processes, while retaining positive pool_size validation. Set auto_suspend to 14400 seconds consistently across function creation, catalog metadata, Thrift, and backend defaults. Require explicit -1 to disable idle reclamation and reject smaller values. ### Release note Python function pools are capped by each backend's CPU core count and are reclaimed after four hours without active clients by default. The next call recreates the pool. Explicit auto_suspend=-1 disables idle reclamation. ### Check List (For Author) - Test: Full build; 38 FE unit tests; 29 BE unit tests on both host and one-core configurations; four targeted regression suites; all 19 Python UDTF suites; verified pool_size=10000 starts exactly one Python server on a one-core BE; clang-format 16 and clang-tidy checks. - Behavior changed: Yes, cap effective pool_size per backend and default auto_suspend to four hours; reject auto_suspend values below -1. - Does this need documentation: Yes, English and Chinese development-version descriptions are updated in the separate doris-website working directory.
linrrzqqq
force-pushed
the
pyudf-process-pool
branch
from
October 23, 2026 09:26
1118af0 to
eee46a1
Compare
Collaborator
Author
|
/review |
Collaborator
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Static review of eee46a1: one new P2 finding, a deterministic failure of multi-worker BE tests on a host/container with one effective CPU. No additional production defect was established beyond existing review threads. All 151 changed paths and their relevant callers, teardown, replay, and test paths were covered. No additional user-provided focus was specified.
Existing-thread disposition: module entrypoint collisions, late-query generation recreation/package deletion, and cleanup intent lost on FE failover still apply and are not reposted. Head now defaults auto_suspend to 14400 seconds; explicit -1 still permits indefinite retention in the relevant cleanup scenarios. The prior database retry and ordinary/global DROP retry gaps are addressed by queued generation-scoped tasks and acknowledgements. No existing P0/P1 finding was confirmed.
Critical checkpoint conclusions:
- Goal and evidence: function-id pools, native package caching, resource properties, and same-master cleanup retries are implemented with unit/regression coverage; remaining lifecycle/import concerns are the existing threads above.
- Scope and reuse: changes are coherent across FE metadata, BE ownership, and tests; FunctionUtil centralizes cleanup dispatch.
- Concurrency and locks: acquisition leases protect initialization/use/close; map-before-pool ordering is consistent, registry snapshots release their lock before further work, and fork/shutdown work occurs outside map locks. No additional deadlock or active-client reclamation issue was verified.
- Lifecycle and static initialization: shared handles pin packages/processes, weak registry entries avoid cycles, and STOPPED rejects new clients while retained. Permanent retirement remains incomplete as already reported; no new cross-TU initialization dependency was found.
- Configuration: pool_size and auto_suspend are per-function metadata, preserved through copies and persistence. The removed global process-count setting has no remaining source/config references. FE/BE defaults agree at 3/14400; PR prose describing -1 as the default is stale.
- Compatibility: optional-field fallbacks agree. Old BEs execute tracked cleanup but retain bookkeeping until their upgrade restart allows resend/acknowledgement; no distinct query correctness defect was established from this limitation.
- Thrift/protobuf: complete TFunction definitions were compared on HEAD and current branch-3.1/4.0/4.1/4.2 refs, verified against the remote. IDs 20/21 are unused across those branches and both additions are optional; no protobuf changes.
- Parallel paths: scalar/UDAF/UDTF and outer-function metadata, local/global DROP, force/expired/same-name/instant database erasure, and cloud/non-cloud backend enumeration were checked.
- Conditions and errors: invalid pool/timeout values are rejected; invalid cleanup IDs report failure; empty cleanup sets avoid signature fallback. Lease cleanup also runs after writer errors. No additional changed-path error-propagation defect was verified.
- Test coverage: property validation, copies, Gson/Thrift, idle/PID reclamation, package isolation, dropped-active memory, retries, lost acknowledgements, and ordinary/global DROP are represented. The new CPU cap is not reflected in every multi-worker unit-test expectation, as reported inline. Existing late-open/failover gaps remain tied to the existing findings.
- Test results: new golden outputs match their SQL and unchanged null/outer semantics on inspection. The ZIP adds exactly the two matching module-cache sources; existing members are unchanged. Execution and artifact-generation provenance were not verified.
- Observability: existing RSS tracking now includes retained processes; initialization/cleanup/retry logs include relevant identifiers and use existing task metrics. No additional required metric/log defect was established.
- Persistence and failover: function fields are serialized, copied, and replayed through existing metadata paths. Cleanup intent remains volatile across failover, as already reported.
- Data writes and crashes: transaction visibility, rowset versions, and delete bitmaps are unchanged. Crash-related cleanup retention is covered by the existing failover concern.
- FE-BE variables: both converters, all Python planner reconstruction/copy routes, and all three BE entry points carry/default the new properties consistently. Return/null/aggregate intermediate representations are unchanged.
- Performance: per-function process scaling is intentional and capped by effective CPUs; cleanup scales with removed generations and backends. No additional avoidable hot-path regression was substantiated.
- Final recheck and limits: candidate deduplication, package ownership, detached workers, locks, tests, and special paths received an additional pass. This review is static-only: no builds, tests, cluster runs, or source modifications were performed. Runtime behavior and final binary ABI remain unvalidated.
Review converged after 2 round(s).
| auto first_pool = mgr.process_pool_snapshot_for_test(101); | ||
| auto second_pool = mgr.process_pool_snapshot_for_test(102); | ||
| ASSERT_EQ(first_pool.size(), 1); | ||
| ASSERT_EQ(second_pool.size(), 2); |
Contributor
There was a problem hiding this comment.
[P2] Make multi-worker tests respect the CPU pool cap
On a one-CPU host or cgroup-limited runner, the new min(meta.pool_size, CpuInfo::num_cores()) creates only one process for second, so this unconditional size-2 assertion fails even though the pool behaves correctly. The adapted EnsurePoolInitializedSucceedsWithOneStuckWorkerAndOneUsableWorker also now launches only its deliberately stuck worker and fails initialization; the previous explicit global limit of 2 allowed both workers. Derive sizes/indexing from the effective pool size, and skip or control the CPU count for scenarios that require at least two workers.
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 28769 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-DS: Total hot run time: 150827 ms |
Contributor
ClickBench: Total hot run time: 24.45 s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Problem Summary:
Refactor Python UDF process scheduling to maintain an independent process pool for each function id, simplifying process allocation, package isolation and lifecycle management for Python UDF, UDAF and UDTF.
As part of the refactoring, add two function properties:
pool_size: maximum number of Python processes per function on each BE, defaulting to3.auto_suspend: idle timeout in seconds, defaulting to-1to keep pools resident. Nonnegative values allow idle pools to be terminated during the 30-second monitor scan and recreated on the next invocation.Both properties are persisted in function metadata and currently supported only for Python functions.
Adapt cleanup to the new pool ownership:
Release note
Python functions use independent process pools. The pool_size and auto_suspend properties control pool capacity and idle reclamation.
Check List (For Author)