Copilot commented on code in PR #17842:
URL: https://github.com/apache/pinot/pull/17842#discussion_r2909316057
##########
pinot-controller/src/main/java/org/apache/pinot/controller/validation/BrokerResourceValidationManager.java:
##########
@@ -55,15 +69,21 @@ protected Context preprocess(Properties
periodicTaskProperties) {
@Override
protected void processTable(String tableNameWithType, Context context) {
TableConfig tableConfig =
_pinotHelixResourceManager.getTableConfig(tableNameWithType);
- if (tableConfig == null) {
- LOGGER.warn("Failed to find table config for table: {}, skipping broker
resource validation", tableNameWithType);
+ if (tableConfig != null) {
+ Set<String> brokerInstances = _pinotHelixResourceManager
+ .getAllInstancesForBrokerTenant(context._instanceConfigs,
tableConfig.getTenantConfig().getBroker());
+ _pinotHelixResourceManager.rebuildBrokerResource(tableNameWithType,
brokerInstances);
return;
}
-
- // Rebuild broker resource
- Set<String> brokerInstances = _pinotHelixResourceManager
- .getAllInstancesForBrokerTenant(context._instanceConfigs,
tableConfig.getTenantConfig().getBroker());
- _pinotHelixResourceManager.rebuildBrokerResource(tableNameWithType,
brokerInstances);
+ LogicalTableConfig logicalTableConfig =
_pinotHelixResourceManager.getLogicalTableConfig(tableNameWithType);
+ if (logicalTableConfig != null) {
+ Set<String> brokerInstances = _pinotHelixResourceManager
+ .getAllInstancesForBrokerTenant(context._instanceConfigs,
logicalTableConfig.getBrokerTenant());
+ _pinotHelixResourceManager.rebuildBrokerResource(tableNameWithType,
brokerInstances);
Review Comment:
PR description says `processTable` differentiates physical vs logical
partitions using `TableNameBuilder.isTableResource(...)`, but the
implementation actually differentiates by config lookup (`getTableConfig(...)
!= null` else `getLogicalTableConfig(...)`). Please align the PR description
with the implemented behavior (or update the code if the name-based check is
required) to avoid confusion for future maintainers.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ControllerPeriodicTasksIntegrationTest.java:
##########
@@ -422,6 +422,49 @@ public void testBrokerResourceValidationManager() {
}, 600_000L, "Timeout when waiting for BrokerResourceValidationManager");
}
+ /**
+ * Verifies that BrokerResourceValidationManager also repairs broker
resource for a logical table
+ * when a new broker is added (Issue #15751).
+ */
+ @Test
+ public void testBrokerResourceValidationManagerRepairsLogicalTable()
+ throws IOException {
+ // Add logical table (same broker tenant as physical table)
+ Schema logicalTableSchema = createSchema();
+ logicalTableSchema.setSchemaName(getLogicalTableName());
+ addSchema(logicalTableSchema);
+ createLogicalTable();
+
+ String helixClusterName = getHelixClusterName();
+ String logicalTableName = getLogicalTableName();
+ IdealState idealState = HelixHelper.getBrokerIdealStates(_helixAdmin,
helixClusterName);
+ assertNotNull(idealState);
+ assertTrue(idealState.getPartitionSet().contains(logicalTableName),
"Broker resource should have logical table");
+
+ // Add a new broker so logical table partition is out of sync
+ String brokerId = "Broker_localhost_5678";
+ InstanceConfig instanceConfig = InstanceConfig.toInstanceConfig(brokerId);
+ instanceConfig.addTag(TagNameUtils.getBrokerTagForTenant(TENANT_NAME));
+ _helixAdmin.addInstance(helixClusterName, instanceConfig);
+ Set<String> brokersAfterAdd =
_helixResourceManager.getAllInstancesForBrokerTenant(TENANT_NAME);
+ assertTrue(brokersAfterAdd.contains(brokerId));
+
+ // Wait for BrokerResourceValidationManager to repair both physical and
logical table partitions
+ String tableNameWithType =
TableNameBuilder.OFFLINE.tableNameWithType(getTableName());
+ TestUtils.waitForCondition(aVoid -> {
+ IdealState is = HelixHelper.getBrokerIdealStates(_helixAdmin,
helixClusterName);
+ if (is == null) {
+ return false;
+ }
+ return is.getInstanceSet(tableNameWithType).equals(brokersAfterAdd)
+ && is.getInstanceSet(logicalTableName).equals(brokersAfterAdd);
+ }, 600_000L, "Timeout when waiting for BrokerResourceValidationManager to
repair logical table partition");
+
+ // Cleanup: drop broker and logical table
+ _helixAdmin.dropInstance(helixClusterName, instanceConfig);
+ _helixResourceManager.deleteLogicalTableConfig(logicalTableName);
+ }
Review Comment:
Test adds a logical-table schema via `addSchema(logicalTableSchema)` but
cleanup only drops the broker instance + logical table config. To keep the
integration test cluster clean (and consistent with other tests in this class
that call `deleteSchema(...)`), also delete the logical table schema during
cleanup; otherwise the schema ZNode persists and can accumulate across runs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
