[improvement](fe) Balance runtime filter coordinator selection#64130
Open
BiteTheDDDDt wants to merge 1 commit into
Open
[improvement](fe) Balance runtime filter coordinator selection#64130BiteTheDDDDt wants to merge 1 commit into
BiteTheDDDDt wants to merge 1 commit into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR balances runtime filter merge coordinator selection in FE by introducing a shared round-robin selector and ensuring the chosen coordinator is drawn from distinct participating backends rather than always defaulting to the first/top instance. This reduces the chance that repeated queries concentrate coordinator work on a single BE due to stable fragment/worker ordering.
Changes:
- Add a process-wide round-robin helper (
RuntimeFilterCoordinatorSelector) to rotate coordinator selection across queries. - Update Nereids runtime filter thrift building to select the merge worker from all participating workers (deduped by BE).
- Update legacy Coordinator runtime filter merge instance selection to rotate across distinct participating BEs while preserving BE fallback alignment; expand unit tests accordingly.
Review Checkpoints (Part 1.3)
- Goal & correctness: The code changes implement round-robin selection over deduped candidates in both legacy and Nereids paths, matching the described goal.
- Concurrency: Selection state is maintained via
AtomicLong(getAndIncrement()), which is thread-safe for concurrent queries. - Lifecycle/static init: The new static selector is simple and does not introduce cross-class initialization dependencies.
- Tests: Unit tests are added/expanded to validate the selector’s round-robin behavior and Nereids merge-worker selection.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| fe/fe-core/src/test/java/org/apache/doris/qe/runtime/ThriftPlansBuilderTest.java | Expands FE unit tests to cover round-robin selection and Nereids merge-worker selection behavior. |
| fe/fe-core/src/main/java/org/apache/doris/qe/runtime/RuntimeFiltersThriftBuilder.java | Selects merge worker from all distinct participating workers and applies round-robin rotation. |
| fe/fe-core/src/main/java/org/apache/doris/qe/runtime/RuntimeFilterCoordinatorSelector.java | New shared round-robin selector used across coordinator-selection sites. |
| fe/fe-core/src/main/java/org/apache/doris/qe/Coordinator.java | Legacy planner path now round-robins the runtime filter merge instance across distinct participating BEs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
BiteTheDDDDt
force-pushed
the
codex/improve-rf-coordinator-selection
branch
from
ce2a27d to
eea2548
Compare
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Nereids runtime filter merge coordinator selection in FE used the first instance of the top-most distributed plan. The merge coordinator should stay on a top-most fragment instance so its lifetime covers runtime filter merging, but stable plan and worker ordering can still make repeated queries choose the same backend. This change randomly selects the merge worker from the distinct BEs assigned to the top-most distributed plan, so coordinator work is no longer fixed to the first top-most instance while preserving the top-most fragment lifetime requirement. Legacy Coordinator behavior is unchanged.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- ./run-fe-ut.sh --run org.apache.doris.qe.runtime.RuntimeFiltersThriftBuilderTest
- ./build.sh --fe
- Behavior changed: Yes. Nereids runtime filter coordinator selection is randomized across top-most fragment instances instead of using a fixed first top-most instance.
- Does this need documentation: No
BiteTheDDDDt
force-pushed
the
codex/improve-rf-coordinator-selection
branch
from
eea2548 to
c11a45f
Compare
Contributor
Author
|
run buildall |
Contributor
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.
Review result: no blocking issues found.
Critical checkpoint conclusions:
- Goal/test: The PR changes Nereids runtime filter merge coordinator selection from the first top-most fragment instance to a random distinct backend from the top-most distributed plan. This satisfies the stated balancing goal while preserving the merge coordinator on a top-most fragment backend. A focused FE unit test covers candidate collection from the top-most plan and verifies selection is within those candidates.
- Scope/clarity: The change is small and localized to RuntimeFiltersThriftBuilder plus one unit test.
- Concurrency: No shared mutable state or new locking/concurrency path is introduced; ThreadLocalRandom is used only during per-query thrift-plan construction.
- Lifecycle/static initialization: No new static/global lifecycle concerns were introduced.
- Configuration: No configuration items were added.
- Compatibility: No thrift schema, storage format, function symbol, or rolling-upgrade-sensitive protocol change was introduced; only the selected address value changes among already assigned top-most fragment workers.
- Parallel paths: Legacy Coordinator behavior is intentionally unchanged; the modified path is the Nereids ThriftPlansBuilder path that uses RuntimeFiltersThriftBuilder.
- Conditional checks: The new empty-candidate check is an invariant check for malformed distributed plans and is acceptable.
- Test coverage: The added unit test covers the key helper behavior. I attempted to run ./run-fe-ut.sh --run org.apache.doris.qe.runtime.RuntimeFiltersThriftBuilderTest locally, but this runner is missing thirdparty/installed/bin/protoc, so generated source build failed before the test could execute.
- Test results: No .out results are involved.
- Observability: No additional observability appears necessary for this low-level coordinator selection change.
- Transactions/persistence/data writes: Not applicable.
- FE-BE variable passing: Existing runtime filter params propagation is preserved; merge params are still populated only for the backend matching runtime_filter_merge_addr, and the selected backend is drawn from the top-most plan workers so it is present in the thrift output.
- Performance: Candidate collection is linear in the number of top-most instances and negligible compared with plan construction; no hot-path regression found.
User focus: No additional user-provided review focus was specified.
Contributor
TPC-H: Total hot run time: 29251 ms |
Contributor
TPC-DS: Total hot run time: 168863 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
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
Related PR: None
Problem Summary: Nereids runtime filter merge coordinator selection in FE used the first instance of the top-most distributed plan. The merge coordinator should stay on a top-most fragment instance so its lifetime covers runtime filter merging, but stable plan and worker ordering can still make repeated queries choose the same backend. This change randomly selects the merge worker from the distinct BEs assigned to the top-most distributed plan, so coordinator work is no longer fixed to the first top-most instance while preserving the top-most fragment lifetime requirement. Legacy Coordinator behavior is unchanged.
Release note
None
Check List (For Author)