Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
|
Superseded by #68199, which backports the whole local shuffle series as one PR. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Static review of exact head 0d52d03055a0638d7028b5fd02e1d9140d94b1f2 found three actionable issues: a pooled non-bucket exchange can leave receivers waiting forever, the exact-head CheckStyle gate fails on two unused imports, and a valid one-thread executor cap can be replaced with CPU cores and trigger pure-cost exchanges.
Critical checkpoints:
- Scope and intent: reviewed all 17 authoritative changed files plus fragment construction, distribution assignment, receiver/task creation, runtime-filter publication, worker-cap reporting, configuration, and test call chains.
- Planner/distribution correctness: bucket destination ownership and current-version bucket receiver EOS accounting are consistent, but the separate non-bucket RANDOM path is not.
- Runtime/concurrency/lifecycle: zero-sender dependency readiness and runtime-filter local/global manager symmetry were checked; no additional issue survived.
- Configuration/performance: threshold/default forwarding is consistent; the reported one-executor fallback is not.
- Protocol/compatibility: optional Thrift behavior and supported upgrade ordering were checked; no additional comment survived.
- Tests/static gates: the new regression can skip feature-shape assertions in unsuitable environments, and live CheckStyle confirms the two reported import errors. No build or test was run locally because the review contract prohibited execution.
- Security, authorization, transactionality, persistence, and data-write semantics: not applicable to these changes.
- User focus: no additional focus was provided.
- Convergence: after three bounded rounds, every normal and risk-focused lane returned
NO_NEW_VALUABLE_FINDINGSwith this three-comment set unchanged.
The PR became closed during the review, but its live base/head still exactly match the authoritative bundle; this review records the findings against that exact commit.
|
|
||
| @Override | ||
| public boolean isSerialOperatorOnBe(ConnectContext context) { | ||
| if (context != null && context.getSessionVariable().isEnableLocalShufflePlanner()) { |
There was a problem hiding this comment.
[P1] Preserve serial handling for funneled non-bucket exchanges
Removing the fragment-level serial-scan condition here makes a RANDOM exchange in a pooled scan fragment non-serial. Mixed partitioned/unpartitioned set operations create that shape: the partitioned scan child is absorbed, while the unpartitioned child remains behind a RANDOM exchange. DistributePlanner still sends every non-bucket input only to getFirstInstancePerWorker(), and the new zero-sender completion only recognizes bucket shuffle. The other local tasks therefore construct receivers with the full sender count but get no channel or EOS, so the query can wait forever. Please keep these funneled exchanges serial, or spread and terminate their receivers as well.
| import com.google.common.base.Preconditions; | ||
| import com.google.common.collect.ImmutableList; | ||
| import com.google.common.collect.Lists; | ||
| import org.apache.logging.log4j.LogManager; |
There was a problem hiding this comment.
[P1] Remove the unused imports that fail CheckStyle
Neither LogManager nor Logger is referenced in this file. The exact-head CheckStyle job already rejects these two added lines (and reports no other Checkstyle errors), so this PR cannot pass the required gate until both imports are removed or used.
| org.apache.doris.system.Backend backend = | ||
| ((org.apache.doris.nereids.trees.plans.distribute.worker.BackendWorker) worker).getBackend(); | ||
| int size = backend.getPipelineExecutorSize(); | ||
| if (size <= 1) { |
There was a problem hiding this comment.
[P2] Honor a reported single pipeline executor
pipeline_executor_size = 1 is a valid configured capacity: the BE reports every positive value verbatim, but this branch treats one as ‘not reported’ and replaces it with the machine CPU count. On a many-core BE intentionally configured with one pipeline executor, the ratio can therefore enable two local hash exchanges even though effective concurrency remains one, making this default-on optimization pure overhead. Please distinguish the unreported state, or conservatively preserve any positive reported executor size.
2 participants
Cherry-picked from #64793