[Feature](functions) add brokers tvf function to query broker cluster info#60323
Open
heguanhui wants to merge 1 commit into
Open
[Feature](functions) add brokers tvf function to query broker cluster info#60323heguanhui wants to merge 1 commit into
heguanhui wants to merge 1 commit into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
heguanhui
force-pushed
the
feature/support-brokers-table-value-function
branch
from
6a762b4 to
524c19c
Compare
heguanhui
changed the title
Contributor
Author
|
run buildall |
heguanhui
force-pushed
the
feature/support-brokers-table-value-function
branch
from
524c19c to
9fce5e8
Compare
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 32499 ms |
ClickBench: Total hot run time: 27.75 s |
BE 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
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 31979 ms |
ClickBench: Total hot run time: 27.81 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
heguanhui
force-pushed
the
feature/support-brokers-table-value-function
branch
from
7147271 to
fc86655
Compare
Contributor
Author
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 32413 ms |
ClickBench: Total hot run time: 27.66 s |
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 32259 ms |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
ClickBench: Total hot run time: 27.94 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
ClickBench: Total hot run time: 25.74 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
heguanhui
force-pushed
the
feature/support-brokers-table-value-function
branch
3 times, most recently
from
19b7c92 to
7baf965
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29491 ms |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-DS: Total hot run time: 173353 ms |
Contributor
ClickBench: Total hot run time: 25.98 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found several issues that should be fixed before merge.
Critical checkpoint conclusions:
- Goal/test: The PR adds a
brokers()TVF and tests, but the implementation does not fully match existing broker metadata auth or name semantics, and the tests miss important edge cases. - Scope: The feature is small, but it crosses broker metadata authorization and FE/BE metadata-scan compatibility boundaries.
- Concurrency/lifecycle: No new concurrency or lifecycle state issue found; broker reads still use existing
BrokerMgrpaths. - Configuration/persistence/writes: No new configs or persisted state found.
- Compatibility: The new metadata enum can silently return empty rows on old BEs during rolling upgrade.
- Parallel paths:
SHOW BROKERandSHOW PROC '/brokers'remain stricter than the TVF, and exact broker-name APIs conflict with the TVF's case-insensitive filter behavior. - Testing: Current tests cover happy paths but miss uppercase-key filtering, case-distinct broker names, auth parity, and unsupported metadata behavior.
- Observability/performance: No separate observability or performance issue found beyond the existing metadata scan path.
Subagent conclusions: MAIN-2 came from TEST-1; MAIN-3 and MAIN-4 came from OPT-1 and OPT-2; MAIN-1 was added by the main review. Both live subagents completed convergence round 1 with NO_NEW_VALUABLE_FINDINGS for this final comment set.
User focus: no additional user-provided review focus.
Validation: static review only. This checkout lacks thirdparty/installed and thirdparty/installed/bin/protoc, so I did not run FE/BE builds or regression tests.
| if (params == null) { | ||
| throw new AnalysisException("params cannot be null"); | ||
| } | ||
| String originalClusterName = params.get(CLUSTER_NAME); |
Contributor
There was a problem hiding this comment.
CLUSTER_NAME is accepted by the validation loop below, but this lookup is case-sensitive and runs before any normalization. For brokers("CLUSTER_NAME" = "missing"), originalClusterName stays null, getMetaScanRange() sends a null filter, and the scan returns all brokers instead of zero rows. The new regression case with uppercase CLUSTER_NAME does not catch this because it uses the existing test cluster and expects all three rows. Please normalize the property map before lookup, as the other metadata TVFs do, and add a case where uppercase CLUSTER_NAME must filter something out.
Contributor
Author
There was a problem hiding this comment.
I fixed it already.
| if (StringUtils.isBlank(originalClusterName)) { | ||
| throw new AnalysisException("Invalid brokers param value: " + originalClusterName); | ||
| } | ||
| clusterName = originalClusterName.toLowerCase(); |
Contributor
There was a problem hiding this comment.
This changes the semantics of broker names from exact to case-insensitive only for the TVF. Add/drop and lookup paths preserve the broker name text and use exact map keys in BrokerMgr, so BrokerA and brokera can be distinct broker names. After this lowercasing, MetadataGenerator.brokersMetadataResult() compares with equalsIgnoreCase, so brokers("cluster_name" = "BrokerA") would return both clusters. Please preserve the original filter value and compare broker names consistently with the existing broker manager APIs.
Contributor
Author
There was a problem hiding this comment.
I fixed it already.
| + " only support property [" + CLUSTER_NAME + "]"); | ||
| } | ||
| } | ||
| if (!Env.getCurrentEnv().getAccessManager().checkDbPriv(ConnectContext.get(), |
Contributor
There was a problem hiding this comment.
This privilege check is weaker than the existing broker metadata surfaces. SHOW BROKER gates the same BrokerMgr.getBrokersInfo() data behind global ADMIN or OPERATOR/NODE, and SHOW PROC '/brokers' requires ADMIN_OR_NODE, but this TVF only requires SELECT on internal.information_schema. New users get that information_schema SELECT in their default role, so select * from brokers() would expose broker names, hosts, ports, liveness, and errors to users who cannot run SHOW BROKER. Please align the TVF auth with the existing broker command/proc privilege before returning these rows.
Contributor
Author
There was a problem hiding this comment.
I fixed it already.
| PAIMON = 12, | ||
| PARQUET = 13, | ||
| STREAMS = 14, | ||
| BROKERS = 15, |
Contributor
There was a problem hiding this comment.
This new metadata type needs a compatibility gate or a failure path for mixed FE/BE versions. A new FE will build a TMetaScanRange with BROKERS and MetadataScanNode sends it to whatever query backend FederationBackendPolicy selects. If that BE is still old during rolling upgrade, it does not have the BROKERS case and MetaScanner::_fetch_metadata() takes the default path that sets EOF and returns OK, so brokers() can silently return no rows. Please either route/gate this TVF to BEs that support the new type or make unsupported metadata types fail loudly instead of succeeding with an empty result.
Contributor
Author
There was a problem hiding this comment.
Thanks for the suggestion. However, currently none of the other TVFs handle upgrade compatibility either. To keep consistency with existing implementations, we won’t address it separately in this PR for now. If there is a unified upgrade compatibility solution in the future, we can support it then.
yx-keith
reviewed
Jul 6, 2026
Contributor
There was a problem hiding this comment.
brokersMetadataResult() is missing the ADMIN check that backends()/frontends() do in MetadataGenerator — so a SELECT-only user can read broker info here, while SHOW BROKER requires ADMIN/OPERATOR. Please add checkGlobalPriv(currentUserIdentity, PrivPredicate.ADMIN) (return empty for non-admin) as the sibling TVFs do, plus a non-admin regression case.
Also NonConcurrent Regression is red — please confirm it's unrelated before merge.
heguanhui
force-pushed
the
feature/support-brokers-table-value-function
branch
2 times, most recently
from
bc1db5a to
7461c94
Compare
Contributor
Author
|
run buildall |
heguanhui
force-pushed
the
feature/support-brokers-table-value-function
branch
from
7461c94 to
ed246ba
Compare
heguanhui
force-pushed
the
feature/support-brokers-table-value-function
branch
from
ed246ba to
7fe7a59
Compare
Contributor
Author
|
run buildall |
1 similar comment
Contributor
|
run buildall |
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Automated review for PR #60323.
Requested changes:
- The new brokers TVF regression test is not isolated from global broker state; see the inline comment.
Critical checkpoint conclusions:
- Goal: the PR adds a
brokers()metadata TVF across Nereids registration, catalog TVF construction, FE metadata generation, BE meta scan forwarding, thrift, and tests. The core path is present, but the regression test has a blocking isolation issue. - Scope: the implementation is focused on the new TVF and required metadata plumbing.
- Concurrency/lifecycle: broker rows are read through
BrokerMgr.getBrokersInfo()under the existing broker-manager lock; no new lifecycle or lock ordering issue was found. - Configuration/compatibility: no new config item. The mixed FE/BE metadata enum compatibility concern is already covered by existing thread 3526269320, so I did not duplicate it.
- Parallel paths/protocol: checked Nereids binding/visitor registration,
TableValuedFunctionIf,MetadataScanNode, BEMetaScanner, FEMetadataGenerator, and thrift fields. The broker-name exactness issue is already covered by existing thread 3526269314. - Tests: FE unit and regression coverage were added, but the regression suite currently assumes no unrelated brokers exist globally. I could not run FE/BE tests in this runner because
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protocare missing in the shallow checkout. - User focus: no additional user-provided focus was specified.
Subagent conclusions:
optimizer-rewrite:OPT-1was merged as duplicate of existing thread 3526269314; no new non-duplicate optimizer/rewrite finding.tests-session-config:TEST-2was accepted as the inline comment below;TEST-1was merged as duplicate of existing thread 3526269314.- Convergence Round 1 ended with both live subagents returning
NO_NEW_VALUABLE_FINDINGSfor the same final ledger/comment set.
| def columnsExpr = String.join(",", columns) | ||
| log.info("columns expr: ${columnsExpr}") | ||
|
|
||
| def brokers = sql """select ${columnsExpr} from brokers()""" |
Contributor
There was a problem hiding this comment.
This makes the suite depend on the whole FE broker registry being empty before it owns any state. brokers() and show broker return global broker metadata, while other regression suites and manual/debug runs can leave unrelated broker names behind. In that case these initial size() == 0 assertions fail even though the new TVF works for this test's broker cluster. Please make the setup and assertions scoped to brokers_tvf_test_cluster: drop that name before starting, assert brokers("cluster_name" = brokerClusterName) is empty before add, and avoid asserting that the entire broker registry has no rows.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 35684 ms |
Contributor
ClickBench: Total hot run time: 25.68 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
4 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.
What problem does this PR solve?
add brokers tvf function to query broker cluster info
1、same performance as show broker command
2、same performance as show proc '/brokers' command
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)