Nightly upstream merge - #39
Conversation
Submitting a task creates the platform thread that runs it. On a worker running thousands of drivers, thread creation can be slow enough that holding the lifecycle lock stalls unrelated submissions and prevents the worker from shedding load. Keep group creation and removal serialized with close, but let submit rely on the volatile closed check. A submission racing with close is rejected by the executor or observes the finished scheduling queue.
Starting a split creates its platform thread. Holding the task executor or task monitor across that operation blocks task registration, removal, and split completion while a loaded worker is least able to make progress. Claim leaf splits under the bookkeeping locks and start them afterward. Requeue claims when thread creation fails so the periodic pass can retry them. Complete split futures and tear drivers down outside task monitors, including races with task removal and failures from driver close.
Periodic concurrency adjustment, diagnostics, and JMX reads race with request threads adding and removing tasks. Copying a guarded HashMap avoids corruption but repeatedly takes the global monitor and nests it with every task monitor. Use a ConcurrentHashMap so maintenance can iterate a weakly consistent view without copying or taking the task executor lock. Iterate leaf scheduling in rounds to retain its existing task distribution.
A task scheduled with scheduleWithFixedDelay is silently dropped if it throws, so one thread-creation failure can permanently stop leaf split scheduling or per-task concurrency adjustment. Keep maintenance tasks registered, coalesce concurrent global scheduling passes and rate-limit repeated failure logs. Split completion first tops up the task that released a slot, then runs a global pass when that task drains. Temporary start failures are requeued for the next pass.
The target and source AssignUniqueId operators of a MERGE can execute in the same task and drew overlapping ids from separate pools, causing false MERGE_TARGET_ROW_MULTIPLE_MATCHES failures.
85feebc replaced JoinCondition.Operator.IS_DISTINCT_FROM with IDENTICAL and gave it the IR's display symbol "≡" as its value. DefaultQueryBuilder.formatJoinCondition splices that value straight into the generated SQL, so an IS NOT DISTINCT FROM join pushed into a connector that accepts it (PostgreSQL, Ignite) produced `l."x" ≡ r."y"` and failed with a remote syntax error. The path is reached whenever complex_join_pushdown_enabled is off. With the value fixed, the test-side workarounds that skipped IDENTICAL and spelled the operator out by hand are no longer needed.
The cache asks the delegate for the file length on every access, which is a remote round trip that the existing assertions did not count.
benben
left a comment
benben
left a comment
There was a problem hiding this comment.
Automated review generated on behalf of @benben.
Approve. Clean fast-forward-style merge of upstream trinodb/trino master into the fork: no file is touched by both sides, and the merge result equals the upstream diff exactly. The upstream changes (lock-free split start in ThreadPerDriverTaskExecutor, shared AssignUniqueId pool, IDENTICAL SQL text, immutable StageInfo.subStages) trace correctly; the destroy/claim/start races are handled and the production DriverSplitRunner tolerates start-after-close as the code assumes. Note the PR's local validation only built trino-ducklake, not core/trino-main where all substantive changes land.
- minor
core/trino-main/src/main/java/io/trino/execution/executor/dedicated/ThreadPerDriverTaskExecutor.java:222Freed global leaf slot idles up to 100ms when finishing task is capped. Task hits its own concurrency cap but still has pending splits while another task is under its cap; no global pass runs until the periodic one.
Model: fable.
Co-authored-by: Shelley <shelley@exe.dev>
benben
left a comment
benben
left a comment
There was a problem hiding this comment.
Automated review generated on behalf of @benben.
Approve (re-review after new commits, previously approve). The only new commit swaps the MinIO KES test image from Docker Hub to Quay, same tag, passed straight to GenericContainer which accepts registry-qualified names. The upstream merge content is unchanged from the previously approved review.
Earlier findings: The minor ThreadPerDriverTaskExecutor global-slot idling finding was not addressed and still stands; nothing else was previously raised.
- nit
plugin/trino-exchange-filesystem/src/test/resources/readme.txt:4Readme still documents Docker Hub minio/kes image for cert regeneration. Following the readme's docker run command now hits the 404 repository.
Model: fable.
8 participants
Merge current
trinodb/trinomaster into the PostHog fork.CI compatibility fix:
Local validation:
JAVA_HOME=/usr/lib/jvm/jdk-25.0.4.1+1 ./mvnw install -pl plugin/trino-ducklake -am -DskipTests -Dair.check.skip-all=true