[feature](fe) Add resource group affinity extension framework#65173
Open
deardeng wants to merge 3 commits into
Open
[feature](fe) Add resource group affinity extension framework#65173deardeng wants to merge 3 commits into
deardeng wants to merge 3 commits into
Conversation
deardeng
requested review from
CalvinKirs,
dataroaring,
gavinchou,
liaoxin01,
luwei16 and
morningman
as code owners
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
deardeng
force-pushed
the
resource-group-impl-master
branch
2 times, most recently
from
10b4692 to
01868c3
Compare
Collaborator
Author
|
run buildall |
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.
Automated review summary:
I found three issues that need changes before this should land. The main problems are that two newly exposed FE configs are not wired into the runtime paths they describe, and the prepared point-query fast path does not preserve the new query affinity semantics.
Critical checkpoint conclusions:
- Goal/test proof: the no-op public framework is mostly scoped, but the extension/config contract is incomplete in query and repair paths. Unit tests cover basic no-op/session validation, but not the disabled config paths or prepared point-query reuse.
- Scope/focus: the change is generally focused on FE resource-group affinity plumbing; no existing inline threads or user focus items needed duplicate suppression.
- Concurrency/lifecycle: no new FE locking issue found in the reviewed heartbeat, ServiceLoader, and scheduler paths.
- Config/compatibility: optional thrift fields are compatible, but
resource_tag_location_checkandenable_repair_src_replica_local_affinitycurrently do not control behavior. - Parallel paths: query scan, prepared point query, stream/mysql/group-commit load, repair scheduling, frontends metadata, audit, and metrics paths were reviewed. The point-query path needs special handling because it reuses a cached scan node and shuffles candidate backends.
- Validation: static review plus
git diff --checkon the correct one-commit PR range. I did not run FE tests/build because this checkout is missingthirdparty/installed/bin/protoc, andfe/AGENTS.mdrequires stopping FE build setup in that case.
Subagent conclusions: optimizer-rewrite OR-1 and OR-2 were merged into M-1 and submitted as one inline comment. tests-session-config TS-1 became M-2 and TS-2 became M-3. No candidates were dismissed or duplicate-suppressed. Convergence round 2, after anchor repair, ended with both live subagents returning NO_NEW_VALUABLE_FINDINGS for this exact final comment set.
deardeng
force-pushed
the
resource-group-impl-master
branch
from
01868c3 to
92408bf
Compare
Collaborator
Author
|
/review |
Contributor
There was a problem hiding this comment.
Requesting changes based on four affinity-framework gaps that are still live in the current head.
Critical checkpoint conclusions:
- Goal/test: the PR adds the public Resource Group affinity extension surfaces, but the current code does not fully apply them across query/load scheduling paths. Unit tests cover no-op behavior, session validation, and two config gates, but not the affected cloud load and query scheduling cases below.
- Scope/focus: the framework is mostly localized, but it touches several parallel FE scheduling paths; the remaining issues are exactly in those parallel paths.
- Concurrency/lifecycle: no new FE lock or edit-log lifecycle issue found in this pass. Prepared short-circuit stale affinity was already covered by existing review context and is addressed in the current head.
- Configuration/session propagation: the previously raised config gates are now wired in the current head. Remaining load-affinity session behavior is not propagated to cloud non-group stream/MySQL load placement.
- Compatibility: new thrift fields are optional. I could not run generation/build validation here because
.worktree_initialized,thirdparty/installed, and executablethirdparty/installed/bin/protocare missing. - Parallel paths: issues remain between local vs cloud load paths, legacy vs Nereids scan assignment, and plan-time vs audit-time query affinity decisions.
- Tests/observability: missing tests for effective downstream policies on cloud load, ordinary OLAP scan placement, and statement-level query-affinity audit consistency. The new audit fields currently compute a fresh decision instead of reporting the statement decision.
- Transactions/persistence/data writes/performance: no storage, transaction, or persistence correctness issue found; the main performance concern is semantic scheduling behavior, not overhead.
User focus: no additional user-provided focus was supplied.
Subagent conclusions: tests-session-config proposed TSC-1, accepted as M-2. optimizer-rewrite proposed OR-1 and OR-2 during convergence, accepted as M-3 and M-4. M-1 was found by the main pass. Existing threads 3517152408, 3517152410, and 3517152404 were treated as already-known/stale context and not resubmitted. Convergence completed in round 3 with both optimizer-rewrite and tests-session-config returning NO_NEW_VALUABLE_FINDINGS for the same four-comment set.
Contributor
TPC-H: Total hot run time: 29787 ms |
Contributor
TPC-DS: Total hot run time: 173516 ms |
Contributor
ClickBench: Total hot run time: 25.02 s |
deardeng
force-pushed
the
resource-group-impl-master
branch
from
92408bf to
a71cbb6
Compare
Collaborator
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review against head a71cbb6. I found three correctness issues in the new affinity handling: skip_missing_version replica ordering can be weakened, group-commit backend caching bypasses the effective load-affinity decision, and the default load helper can return an unavailable backend when cloud callers pass unfiltered candidates.
deardeng
force-pushed
the
resource-group-impl-master
branch
from
a71cbb6 to
0675b7e
Compare
Collaborator
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review found two issues that need changes before merge: forwarded SQL can resolve the requester local resource group from stale heartbeat cache, and the frontends() TVF regression expectations were not updated for the new column. I could not run FE/regression tests because thirdparty/installed/bin/protoc is missing in this checkout.
deardeng
force-pushed
the
resource-group-impl-master
branch
from
0675b7e to
c438e64
Compare
deardeng
force-pushed
the
resource-group-impl-master
branch
from
aba1f40 to
37abfb3
Compare
Collaborator
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review summary for PR 65173 at head 37abfb3b58969871e8caec809a1d9eab2a0ce6d1.
I reviewed the current patch, existing inline review threads, changed-file list, FE/Core AGENTS guidance, and the shared subagent ledger. I do not have additional non-duplicate inline comments to add in this pass. The one remaining scan-affinity concern found during review is already covered by the existing Coordinator.java:2518 inline thread, so I did not resubmit it.
Critical checkpoint conclusions:
- Goal and scope: the PR adds a Resource Group affinity SPI and wires query/load/repair scheduling, FE local resource group propagation, audit/frontends observability, session/config/thrift fields, and default no-op behavior. Current head includes fixes for several existing comments around load-affinity gates, audit table schema, frontends() output, invalid compute-group laziness, forwarded local group, and group-commit affinity.
- Size/focus: the change is broad but consistently scoped to FE resource-group affinity and related observability/forwarding surfaces.
- Concurrency/lifecycle: no new lock-order or heavy-under-lock issue found. The local FE resource group remains transient on
Frontend, with heartbeat refresh plus per-request forwarding to avoid stale requester context. - Configuration/compatibility: new thrift fields are optional; session variables are marked for forwarding and validated; the repair-source and resource-tag gates are wired in current head.
- Parallel paths: stream load, MySQL load, group commit, sink explain, scan planning, point-query lazy evaluation, and coordinator assignment paths were checked. Remaining scan-assignment ordering behavior is duplicate of an existing review thread.
- Tests/results: new FE unit tests and the frontends() regression update cover the changed surfaces that were practical to inspect statically. I ran
bash -n bin/start_fe.sh. Maven/regression tests were not run because this checkout lacksthirdparty/installedandthirdparty/installed/bin/protoc. - Observability: audit text/table fields, frontends() output, SHOW FRONTENDS proc output, and clone affinity metrics were checked for ordering/coverage issues.
- Persistence/transaction/data writes: no edit-log, storage-format, or transaction atomicity issue found in the reviewed changes.
User focus points: no additional user-provided review focus was specified.
Subagent conclusions: optimizer-rewrite reported only OPT-DUP-001, which was merged as duplicate of the existing Coordinator.java:2518 thread; tests-session-config reported no candidates. Final convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the current ledger and empty proposed final comment set.
Collaborator
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 30121 ms |
Contributor
TPC-DS: Total hot run time: 174754 ms |
Contributor
ClickBench: Total hot run time: 25.93 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
deardeng
force-pushed
the
resource-group-impl-master
branch
2 times, most recently
from
961cd32 to
7a141ca
Compare
Issue Number: None
Related PR: None
Problem Summary: Add a public Resource Group affinity extension framework for FE query, load, and repair scheduling paths while keeping the default community implementation as no-op. The change introduces shared policy interfaces, no-op factories, session/config/thrift fields, FE local resource group propagation, and observability fields so downstream implementations can plug in real affinity behavior without changing Apache default scheduling semantics. The audit table schema and AuditLoader buffer now also include the new affinity audit fields so internal.__internal_schema.audit_log stays consistent with the text audit event. EXPLAIN INSERT and group commit load backend selection now use the same load-affinity enable gate as the real load scheduling paths, so disabled load affinity does not call downstream load decision hooks. Forwarded group commit requests only deserialize affinity decisions when the optional affinity fields are present. Scan range planning preserves the invalid-compute-group short-circuit before checking backend membership.
Add a no-op Resource Group affinity extension framework and related compatibility fields.
- Test: Unit Test / Regression test not run
- mvn checkstyle:check -pl fe-core
- mvn test -pl fe-core -am -Dtest=ResourceGroupAffinityNoOpPolicyTest,SessionVariableAffinityTest,OlapScanNodeAffinityConfigTest,TabletSchedCtxTest#testRepairSrcAffinityConfigGate -DfailIfNoTests=false
- mvn -Dmaven.build.cache.enabled=false test -pl fe-core -am -Dtest=InternalSchemaInitializerTest,AuditLoaderTest -DfailIfNoTests=false
- mvn -Dmaven.build.cache.enabled=false test -pl fe-core -am -Dtest=OlapTableSinkAffinityExplainTest -DfailIfNoTests=false
- mvn -Dmaven.build.cache.enabled=false test -pl fe-core -am -Dtest=OlapScanNodeAffinityConfigTest,GroupCommitManagerAffinityTest,MasterOpExecutorAffinityTest,FrontendServiceImplAffinityTest,OlapTableSinkAffinityExplainTest -DfailIfNoTests=false
- git diff --check
- Regression test not run: only updated frontends() TVF assertion/projection for the new column and no local Doris cluster was started.
- Behavior changed: No
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: The OSS extension points for optional backend selection were named after resource group affinity, which exposed enterprise-specific semantics in public FE interfaces. This refactors the OSS boundary to generic backend selection concepts: SelectionHint, Mode, BackendSelectionPolicy, generic session, audit, thrift field names, and generic repair, load, and query selection hooks. The default OSS implementation remains no-op and existing non-cloud and cloud behavior is preserved.
### Release note
None
### Check List (For Author)
- Test:
- Unit Test: ./run-fe-ut.sh --run org.apache.doris.resource.BackendSelectionNoOpPolicyTest,org.apache.doris.qe.SessionVariableBackendSelectionTest,org.apache.doris.qe.MasterOpExecutorBackendSelectionTest,org.apache.doris.service.FrontendServiceImplBackendSelectionTest,org.apache.doris.load.GroupCommitManagerBackendSelectionTest,org.apache.doris.planner.OlapScanNodeBackendSelectionConfigTest,org.apache.doris.planner.OlapTableSinkBackendSelectionExplainTest,org.apache.doris.plugin.audit.AuditLoaderTest,org.apache.doris.qe.PointQueryExecutorTest,org.apache.doris.qe.AuditLogHelperTest,org.apache.doris.load.loadv2.MysqlLoadManagerTest,org.apache.doris.httpv2.rest.LoadActionTest,org.apache.doris.clone.TabletSchedCtxTest,org.apache.doris.qe.CoordinatorTest
- Manual test: mvn checkstyle:check -pl fe-core; ./build.sh --fe; git diff --check
- Behavior changed: No
- Does this need documentation: No
deardeng
force-pushed
the
resource-group-impl-master
branch
from
7a141ca to
830bb78
Compare
Collaborator
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29744 ms |
Contributor
TPC-DS: Total hot run time: 180287 ms |
Contributor
ClickBench: Total hot run time: 25.81 s |
Issue Number: None
Related PR: None
Problem Summary: Fix five issues found in review of the backend selection
extension framework:
1. Restore the documented "random-be" redirect-policy wire value that the
naming refactor accidentally changed to "default-be", which broke
clients (including the built-in AuditStreamLoader) sending the
documented header value.
2. Remove the "require" option from backend_selection_mode and the REQUIRE
enum value. The SPI only lets implementations reorder candidates (never
drop them) and callers keep applying their own load balancing, so a
hard constraint cannot be honored; setting it now fails loudly instead
of being silently ignored.
3. Keep the throwing protocol for forwarded group-commit backend selection
errors unless the follower signals it understands statusCode/errMessage
via the new optional supportsSelectionErrorResult thrift field. An old
follower never reads those fields and would treat the unset
groupCommitLoadBeId as backend 0 during rolling upgrades.
4. Bound the group-commit backend cache with a Guava cache
(maximumSize + expireAfterAccess). Cache keys embed the user-supplied
preference key, so the previous unbounded map could grow without limit
under user-controlled session values.
5. Record the statement-level load selection hint on ConnectContext at the
real scheduling sites (coordinator load path, group commit, forwarded
group commit) and let the audit log prefer it over the scan-side query
decision, so load statements no longer audit an empty or wrong hint.
- Test:
- mvn -Dmaven.build.cache.enabled=false test -pl fe-core -am
-Dtest=LoadActionTest,SessionVariableBackendSelectionTest,
BackendSelectionNoOpPolicyTest,MasterOpExecutorBackendSelectionTest,
FrontendServiceImplBackendSelectionTest,
GroupCommitManagerBackendSelectionTest,AuditLogHelperTest,
CoordinatorTest -DfailIfNoTests=false
(Tests run: 59, Failures: 0, Errors: 0)
- mvn checkstyle:check -pl fe-core
- git diff --check
- Behavior changed: Yes, backend_selection_mode no longer accepts
"require"; redirect-policy "random-be" works again as documented.
- Does this need documentation: No
deardeng
force-pushed
the
resource-group-impl-master
branch
from
5102af5 to
c03d059
Compare
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
2 participants
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.
Problem Summary: Add a public Resource Group affinity extension framework for FE query, load, and repair scheduling paths while keeping the default community implementation as no-op. The change introduces shared policy interfaces, no-op factories, session/config/thrift fields, FE local resource group propagation, and observability fields so downstream implementations can plug in real affinity behavior without changing Apache default scheduling semantics.
What problem does this PR solve?
Issue Number: None
Related PR: None
Check List (For Author)
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)