J-HowHuang commented on code in PR #16964:
URL: https://github.com/apache/pinot/pull/16964#discussion_r2408647926
##########
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
`tableConfig.getTableName()` returns tableNameWithType I believe.
> Have you verified that all callers actually want to look at the instances
that are actually assigned and not check the instances tag list?
1. They are directly use by controller API, e.g. `GET
/tables/{tableName}/instances`, `GET /debug/tables/{tableName}`. These are
affected by what instances will be shown in the API response. This PR change is
legit for this usage.
2. It's used in `ResourceUtilizationManager`, to get on which instances to
check `isDiskUtilizationWithinLimits`. This PR change is legit for this usage
because we want to check whether the instances "hosting" the table has safe
resource util.
3. It's used in `TableMetadataReader#getStaleSegments` to get instance to
send a server side API `GET /tables/{tableName}/segments/isStale`. This also
needs to be the servers that host the segment, so that they have the
`TableDataManager` instance to answer the API.
> Also is there any value in having a new API that does return the instances
based on the tag?
If any, I think we should make `getInstancePartitions` methods in
`TableRebalancer` static and moved to
`org/apache/pinot/controller/helix/core/assignment/instance` or
`org/apache/pinot/common/assignment`, so that it can be used across Pinot. This
way we can get the instances that the rebalancer would rebalance to, instead of
implementing the tag, tagoverride, tier config and instance assignment config
and what not on the side.
--
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]
