Jackie-Jiang commented on code in PR #11241:
URL: https://github.com/apache/pinot/pull/11241#discussion_r1283970208
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -299,16 +274,40 @@ private List<TableConfig>
getListTableConfigs(List<String> tableNames) {
return allTableConfigList;
}
- // return the brokerTenant if all table configs point to the same broker,
else returns null
- private String getCommonBrokerTenant(List<TableConfig> tableConfigList) {
- Set<String> tableBrokers = new HashSet<>();
- for (TableConfig tableConfig : tableConfigList) {
- tableBrokers.add(tableConfig.getTenantConfig().getBroker());
+ private String selectRandomInstanceId(List<String> instanceIds) {
+ if (instanceIds.isEmpty()) {
+ return QueryException.BROKER_RESOURCE_MISSING_ERROR.toString();
+ }
+
+ instanceIds.retainAll(_pinotHelixResourceManager.getOnlineInstanceList());
+ if (instanceIds.isEmpty()) {
+ return QueryException.BROKER_INSTANCE_MISSING_ERROR.toString();
}
- if (tableBrokers.size() != 1) {
- return null;
+
+ // Send query to a random broker.
+ return instanceIds.get(RANDOM.nextInt(instanceIds.size()));
+ }
+
+ private List<String> findCommonBrokerInstance(Set<String> brokerTenants) {
Review Comment:
```suggestion
private List<String> findCommonBrokerInstances(Set<String> brokerTenants) {
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -469,6 +469,11 @@ public List<InstanceConfig>
getBrokerInstancesConfigsFor(String tableName) {
TagNameUtils.getBrokerTagForTenant(brokerTenantName));
}
+ public List<InstanceConfig> getAllBrokerInstanceConfigs() {
+ return HelixHelper.getInstanceConfigs(_helixZkManager).stream()
+ .filter(instance ->
InstanceTypeUtils.isBroker(instance.getId())).collect(Collectors.toList());
+ }
Review Comment:
We can add API to get all broker instance ids which is much cheaper
```suggestion
public List<String> getAllBrokerInstances() {
return HelixHelper.getAllInstances(_helixAdmin, _clusterName).stream()
.filter(instanceId ->
InstanceTypeUtils.isBroker(instanceId)).collect(Collectors.toList());
}
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -299,16 +274,40 @@ private List<TableConfig>
getListTableConfigs(List<String> tableNames) {
return allTableConfigList;
}
- // return the brokerTenant if all table configs point to the same broker,
else returns null
- private String getCommonBrokerTenant(List<TableConfig> tableConfigList) {
- Set<String> tableBrokers = new HashSet<>();
- for (TableConfig tableConfig : tableConfigList) {
- tableBrokers.add(tableConfig.getTenantConfig().getBroker());
+ private String selectRandomInstanceId(List<String> instanceIds) {
+ if (instanceIds.isEmpty()) {
+ return QueryException.BROKER_RESOURCE_MISSING_ERROR.toString();
+ }
+
+ instanceIds.retainAll(_pinotHelixResourceManager.getOnlineInstanceList());
+ if (instanceIds.isEmpty()) {
+ return QueryException.BROKER_INSTANCE_MISSING_ERROR.toString();
}
- if (tableBrokers.size() != 1) {
- return null;
+
+ // Send query to a random broker.
+ return instanceIds.get(RANDOM.nextInt(instanceIds.size()));
+ }
+
+ private List<String> findCommonBrokerInstance(Set<String> brokerTenants) {
+ Set<String> commonInstances = null;
+ for (String brokerTenant : brokerTenants) {
+ Set<String> instances =
_pinotHelixResourceManager.getAllInstancesForBrokerTenant(brokerTenant);
+ if (commonInstances == null) {
+ commonInstances = instances;
+ } else {
+ commonInstances.retainAll(instances);
+ }
+ }
+ return commonInstances == null ? Collections.emptyList() : new
ArrayList<>(commonInstances);
Review Comment:
`commonInstances` can never be `null` here because `brokerTenants` is not
empty
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -299,16 +274,40 @@ private List<TableConfig>
getListTableConfigs(List<String> tableNames) {
return allTableConfigList;
}
- // return the brokerTenant if all table configs point to the same broker,
else returns null
- private String getCommonBrokerTenant(List<TableConfig> tableConfigList) {
- Set<String> tableBrokers = new HashSet<>();
- for (TableConfig tableConfig : tableConfigList) {
- tableBrokers.add(tableConfig.getTenantConfig().getBroker());
+ private String selectRandomInstanceId(List<String> instanceIds) {
+ if (instanceIds.isEmpty()) {
+ return QueryException.BROKER_RESOURCE_MISSING_ERROR.toString();
+ }
+
+ instanceIds.retainAll(_pinotHelixResourceManager.getOnlineInstanceList());
+ if (instanceIds.isEmpty()) {
+ return QueryException.BROKER_INSTANCE_MISSING_ERROR.toString();
}
- if (tableBrokers.size() != 1) {
- return null;
+
+ // Send query to a random broker.
+ return instanceIds.get(RANDOM.nextInt(instanceIds.size()));
+ }
+
+ private List<String> findCommonBrokerInstance(Set<String> brokerTenants) {
+ Set<String> commonInstances = null;
+ for (String brokerTenant : brokerTenants) {
+ Set<String> instances =
_pinotHelixResourceManager.getAllInstancesForBrokerTenant(brokerTenant);
+ if (commonInstances == null) {
+ commonInstances = instances;
+ } else {
+ commonInstances.retainAll(instances);
+ }
+ }
+ return commonInstances == null ? Collections.emptyList() : new
ArrayList<>(commonInstances);
+ }
+
+ // return the union of brokerTenants from the tables list.
+ private Set<String> getBrokerTenantTagsUnion(List<TableConfig>
tableConfigList) {
Review Comment:
```suggestion
private Set<String> getBrokerTenantsUnion(List<TableConfig>
tableConfigList) {
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -299,16 +274,40 @@ private List<TableConfig>
getListTableConfigs(List<String> tableNames) {
return allTableConfigList;
}
- // return the brokerTenant if all table configs point to the same broker,
else returns null
- private String getCommonBrokerTenant(List<TableConfig> tableConfigList) {
- Set<String> tableBrokers = new HashSet<>();
- for (TableConfig tableConfig : tableConfigList) {
- tableBrokers.add(tableConfig.getTenantConfig().getBroker());
+ private String selectRandomInstanceId(List<String> instanceIds) {
+ if (instanceIds.isEmpty()) {
+ return QueryException.BROKER_RESOURCE_MISSING_ERROR.toString();
+ }
+
+ instanceIds.retainAll(_pinotHelixResourceManager.getOnlineInstanceList());
+ if (instanceIds.isEmpty()) {
+ return QueryException.BROKER_INSTANCE_MISSING_ERROR.toString();
}
- if (tableBrokers.size() != 1) {
- return null;
+
+ // Send query to a random broker.
+ return instanceIds.get(RANDOM.nextInt(instanceIds.size()));
+ }
+
+ private List<String> findCommonBrokerInstance(Set<String> brokerTenants) {
+ Set<String> commonInstances = null;
+ for (String brokerTenant : brokerTenants) {
+ Set<String> instances =
_pinotHelixResourceManager.getAllInstancesForBrokerTenant(brokerTenant);
Review Comment:
It will be much faster if we first get all instance configs, and then pass
in the instance configs when getting instance ids. See the `TODO` in
`PinotHelixResourceManager.getAllInstancesForBrokerTenant()`
--
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]
