somandal commented on code in PR #16964:
URL: https://github.com/apache/pinot/pull/16964#discussion_r2408518038
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3562,18 +3562,13 @@ public List<TableConfig>
getTableConfigsForSchema(String schemaName) {
public List<String> getServerInstancesForTable(String tableName, TableType
tableType) {
TableConfig tableConfig = getTableConfig(tableName, tableType);
Preconditions.checkNotNull(tableConfig);
- TenantConfig tenantConfig = tableConfig.getTenantConfig();
Set<String> serverInstances = new HashSet<>();
- List<InstanceConfig> instanceConfigs =
HelixHelper.getInstanceConfigs(_helixZkManager);
- if (tableType == TableType.OFFLINE) {
- serverInstances.addAll(
- HelixHelper.getInstancesWithTag(instanceConfigs,
TagNameUtils.extractOfflineServerTag(tenantConfig)));
- } else if (TableType.REALTIME.equals(tableType)) {
- serverInstances.addAll(
- HelixHelper.getInstancesWithTag(instanceConfigs,
TagNameUtils.extractConsumingServerTag(tenantConfig)));
- serverInstances.addAll(
- HelixHelper.getInstancesWithTag(instanceConfigs,
TagNameUtils.extractCompletedServerTag(tenantConfig)));
+ IdealState idealState = getTableIdealState(tableConfig.getTableName());
Review Comment:
You need to construct and pass the tableNameWithType to fetch the IS here. I
would recommend some manual local testing with quickstart and an actual table
to ensure it works fine for offline, realtime and tiered tables
we had made a similar change for the needsReload API, and had decided to
check EV instead of IS there, since EV is the currently assigned (whereas IS is
desired state but may not be current). Would it make more sense to use EV here
as well?
Have you verified that all callers actually want to look at the instances
that are actually assigned and not check the instances tag list?
Also is there any value in having a new API that does return the instances
based on the tag?
--
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]
