Enhance BrokerResourceValidationManager to support logical table partitions in resource validation by abhishekbafna · Pull Request #17842 · apache/pinot

abhishekbafna · 2026-03-09T16:46:18Z

Summary

This PR updates BrokerResourceValidationManager so that the periodic broker resource validation runs for logical table partitions in the broker resource in addition to physical tables. It closes the gap identified in Issue #15751: after PR #15726, the broker resource includes logical tables, but the validation/repair logic only considered physical tables, so logical table broker assignments were never corrected when brokers were added or removed.

Problem

The broker resource ideal state holds both physical table partitions (tableName_OFFLINE / tableName_REALTIME) and logical table partition names.
BrokerResourceValidationManager previously only used getAllTables() (physical tables), so logical table partitions were never validated or repaired.
When new brokers were added, logical table entries could remain out of sync (missing the new broker); the periodic task did not fix them.

Solution

getTablesToProcess
In addition to physical tables from the base implementation, the task now includes partition names for logical tables by calling PinotHelixResourceManager.getBrokerResourceLogicalTablePartitions(), so both physical and logical broker-resource partitions are processed.
processTable
For each partition name, type is determined by config presence (not by name): look up table config first; if present, treat as physical and resolve broker instances from the table config's broker tenant; otherwise look up logical table config and, if present, resolve from LogicalTableConfig.getBrokerTenant(). Then rebuildBrokerResource is invoked. If neither config exists, the partition is skipped with a warning.
Tests
- BrokerResourceValidationManagerTest (new): unit tests that getTablesToProcess returns both physical tables and logical table partitions, and that the table-name property still restricts to a single table.
- PinotHelixResourceManagerStatelessTest: testUpdateBrokerResourceWithLogicalTable (broker add + rebuild for logical table), testRebuildBrokerResourceFromHelixTagsForLogicalTable (rebuild API for logical table name).
- ValidationManagerStatelessTest: testRebuildBrokerResourceWhenBrokerAddedForLogicalTable (rebuild for logical table after broker add).
- ControllerPeriodicTasksIntegrationTest: testBrokerResourceValidationManagerRepairsLogicalTable (periodic task repairs logical table partition when a new broker is added).

Result

Logical table partitions in the broker resource are now included in the periodic validation and repaired when broker sets change (e.g. new brokers added).
Backward compatible: when PROPERTY_KEY_TABLE_NAME is set, only that table/partition is processed; otherwise physical tables plus logical table partitions are processed.

Pull request overview

Updates broker-resource validation so it also processes logical-table partitions, ensuring broker assignments for logical tables are periodically repaired when broker membership changes (Issue #15751).

Changes:

Extend controller periodic-task plumbing to allow subclasses to customize the table/partition list and processing filter.
Enhance BrokerResourceValidationManager to include logical-table broker-resource partitions and rebuild them using LogicalTableConfig.
Add unit, stateless, and integration tests covering logical-table broker-resource rebuild and periodic repair.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ControllerPeriodicTasksIntegrationTest.java	Adds an integration test verifying periodic validation repairs logical-table broker partitions after adding a broker.
pinot-controller/src/test/java/org/apache/pinot/controller/validation/ValidationManagerStatelessTest.java	Adds stateless test for rebuilding broker resource for a logical table after manual broker addition.
pinot-controller/src/test/java/org/apache/pinot/controller/validation/BrokerResourceValidationManagerTest.java	New unit tests validating logical partitions are included in `getTablesToProcess` behavior.
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerStatelessTest.java	Adds stateless tests for rebuild/update paths involving logical-table partitions.
pinot-controller/src/main/java/org/apache/pinot/controller/validation/BrokerResourceValidationManager.java	Includes logical-table partitions in periodic validation and resolves broker tenants for logical tables.
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/periodictask/ControllerPeriodicTask.java	Introduces overridable hooks for “tables to process” and “should process” filtering.
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java	Adds logical-partition discovery, supports rebuild-by-tags for logical tables, and updates broker ideal state atomically.
pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java	Includes logical tables when mapping broker tags to tables.

...er/src/main/java/org/apache/pinot/controller/validation/BrokerResourceValidationManager.java

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

...er/src/main/java/org/apache/pinot/controller/validation/BrokerResourceValidationManager.java

...rc/test/java/org/apache/pinot/controller/validation/BrokerResourceValidationManagerTest.java

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

codecov-commenter · 2026-03-09T18:32:41Z

Codecov Report

❌ Patch coverage is 56.89655% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.25%. Comparing base (079206a) to head (4f0a013).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...ntroller/helix/core/PinotHelixResourceManager.java	50.00%	12 Missing and 1 partial ⚠️
...er/validation/BrokerResourceValidationManager.java	35.29%	11 Missing ⚠️
...g/apache/pinot/common/utils/helix/HelixHelper.java	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #17842      +/-   ##
============================================
- Coverage     63.27%   63.25%   -0.02%     
- Complexity     1468     1481      +13     
============================================
  Files          3190     3190              
  Lines        192197   192282      +85     
  Branches      29456    29474      +18     
============================================
+ Hits         121612   121628      +16     
- Misses        61060    61120      +60     
- Partials       9525     9534       +9

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (ø)`
integration	`100.00% <ø> (ø)`
integration1	`100.00% <ø> (ø)`
integration2	`0.00% <ø> (ø)`
java-11	`63.20% <56.89%> (-0.02%)`	⬇️
java-21	`63.22% <56.89%> (-0.01%)`	⬇️
temurin	`63.25% <56.89%> (-0.02%)`	⬇️
unittests	`63.25% <56.89%> (-0.02%)`	⬇️
unittests1	`55.57% <0.00%> (-0.01%)`	⬇️
unittests2	`34.27% <56.89%> (-0.02%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 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.

…e validation for logical tables

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

...er/src/main/java/org/apache/pinot/controller/validation/BrokerResourceValidationManager.java

...src/test/java/org/apache/pinot/integration/tests/ControllerPeriodicTasksIntegrationTest.java

krishan1390 · 2026-03-11T06:18:41Z

...er/src/main/java/org/apache/pinot/controller/validation/BrokerResourceValidationManager.java

+      return tables;
+    }
+    List<String> combined = new ArrayList<>(tables);
+    combined.addAll(_pinotHelixResourceManager.getBrokerResourceLogicalTablePartitions());


Can / Should this task run on non-leaders for logical tables ?

Why? Running on the non-leader would get the update from all the controllers.

krishan1390 · 2026-03-11T06:21:11Z

...er/src/main/java/org/apache/pinot/controller/validation/BrokerResourceValidationManager.java

-    _pinotHelixResourceManager.rebuildBrokerResource(tableNameWithType, brokerInstances);
+    LogicalTableConfig logicalTableConfig = _pinotHelixResourceManager.getLogicalTableConfig(tableNameWithType);
+    if (logicalTableConfig != null) {
+      Set<String> brokerInstances = _pinotHelixResourceManager


Why does logical table config have a broker tenant? What happens if a physical table's broker tenant is different from a logical table's broker tenant?

The brokerTenant in the logical table is used to identity the broker node which should be used for the querying. For a logical table, the broker tenant of the physical table can be different and it would not be used, as much I remember.

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java

...test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerStatelessTest.java

...src/test/java/org/apache/pinot/integration/tests/ControllerPeriodicTasksIntegrationTest.java

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

You can also share your feedback on Copilot code review. Take the survey.

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

...ler/src/test/java/org/apache/pinot/controller/validation/ValidationManagerStatelessTest.java

...rc/test/java/org/apache/pinot/controller/validation/BrokerResourceValidationManagerTest.java

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.

You can also share your feedback on Copilot code review. Take the survey.

...rc/main/java/org/apache/pinot/controller/helix/core/periodictask/ControllerPeriodicTask.java

...er/src/main/java/org/apache/pinot/controller/validation/BrokerResourceValidationManager.java

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

...src/test/java/org/apache/pinot/integration/tests/ControllerPeriodicTasksIntegrationTest.java

...ler/src/test/java/org/apache/pinot/controller/validation/ValidationManagerStatelessTest.java

Enhance BrokerResourceValidationManager to support logical table part…

ea66138

…itions in resource validation

abhishekbafna requested review from Jackie-Jiang and Copilot and removed request for Copilot March 9, 2026 16:50

Copilot AI reviewed Mar 9, 2026

View reviewed changes

Copilot started reviewing on behalf of abhishekbafna March 9, 2026 17:09 View session

Refactor BrokerResourceValidationManager to streamline broker resourc…

424bba7

…e validation for logical tables

abhishekbafna requested a review from Copilot March 10, 2026 04:48

Copilot started reviewing on behalf of abhishekbafna March 10, 2026 04:48 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

...er/src/main/java/org/apache/pinot/controller/validation/BrokerResourceValidationManager.java Show resolved Hide resolved

...src/test/java/org/apache/pinot/integration/tests/ControllerPeriodicTasksIntegrationTest.java Outdated Show resolved Hide resolved

abhishekbafna added 2 commits March 10, 2026 10:29

Minor refactoring.

5505e2c

Merge branch 'master' into logocal_table/broker_resources

18cba93

krishan1390 reviewed Mar 11, 2026

View reviewed changes

abhishekbafna added 2 commits March 13, 2026 17:06

Addressing review comments.

4cef7a1

Merge branch 'master' into logocal_table/broker_resources

d76ab55

abhishekbafna requested a review from Copilot March 13, 2026 11:36

Copilot started reviewing on behalf of abhishekbafna March 13, 2026 11:37 View session

Copilot AI reviewed Mar 13, 2026

View reviewed changes

Addressing review comments and refactoring.

a8dc4eb

abhishekbafna requested a review from Copilot March 13, 2026 12:19

Copilot started reviewing on behalf of abhishekbafna March 13, 2026 12:24 View session

Copilot AI reviewed Mar 13, 2026

View reviewed changes

Fixing compilation error.

4f0a013

Conversation

abhishekbafna commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Solution

Result

Related

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

krishan1390 Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

abhishekbafna Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

krishan1390 Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

abhishekbafna Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

abhishekbafna commented Mar 9, 2026 •

edited

Loading

codecov-commenter commented Mar 9, 2026 •

edited

Loading