Skip to content

Add getUnavailableSegments broker hook for route-level unavailable filtering#18906

Merged
9aman merged 1 commit into
apache:masterfrom
KKcorps:external-table-snapshot-unavailable-warning
Jul 3, 2026
Merged

Add getUnavailableSegments broker hook for route-level unavailable filtering#18906
9aman merged 1 commit into
apache:masterfrom
KKcorps:external-table-snapshot-unavailable-warning

Conversation

@KKcorps

@KKcorps KKcorps commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

What

Adds a protected List<String> getUnavailableSegments(BrokerRequest serverBrokerRequest, TableRouteInfo routeInfo) hook to BaseSingleStageBrokerRequestHandler.

  • The default returns routeInfo.getUnavailableSegments() unchanged.
  • Both read sites in handleRequest (the main routing path and the materialized-view-split fallback) now call the hook instead of reading the route directly.

Why

The single-stage handler builds the "segments unavailable" warning from routeInfo.getUnavailableSegments() before any subclass gets to post-process the route in processBrokerRequest. A subclass that prunes routing (e.g. narrowing a query to a single snapshot/version) has no way to keep that warning in sync, so it can report segments the query will never actually read.

This hook is the extension point: a subclass can narrow the reported unavailable segments to the set the query will actually read after routing-level pruning.

Behavior change

None for OSS or existing tables — the default hook returns the same list the code read before.

Testing

Existing pinot-broker tests pass. The hook has no standalone behavior to test beyond the passthrough default, which existing routing/handler tests already exercise.

🤖 Generated with Claude Code

…ltering

Introduce a protected getUnavailableSegments(serverBrokerRequest, routeInfo) hook in
BaseSingleStageBrokerRequestHandler. The default returns routeInfo.getUnavailableSegments()
unchanged, and both read sites (the main routing path and the materialized-view-split fallback)
now route through it. This lets a subclass narrow the reported unavailable segments to those a
query will actually read after routing-level pruning, with no behavior change for OSS or existing
tables.
@codecov-commenter

codecov-commenter commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 64.79%. Comparing base (f0329f3) to head (9cc258b).
⚠️ Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
...sthandler/BaseSingleStageBrokerRequestHandler.java 66.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #18906      +/-   ##
============================================
- Coverage     64.80%   64.79%   -0.01%     
  Complexity     1347     1347              
============================================
  Files          3393     3392       -1     
  Lines        211664   211641      -23     
  Branches      33305    33300       -5     
============================================
- Hits         137160   137135      -25     
- Misses        63431    63444      +13     
+ Partials      11073    11062      -11     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-21 64.79% <66.66%> (-0.01%) ⬇️
temurin 64.79% <66.66%> (-0.01%) ⬇️
unittests 64.79% <66.66%> (-0.01%) ⬇️
unittests1 56.98% <ø> (+<0.01%) ⬆️
unittests2 37.15% <66.66%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@9aman 9aman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Can we add a simple test for this that ensures the filtering part.

@9aman 9aman merged commit bcf3f66 into apache:master Jul 3, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants