yashmayya commented on code in PR #15203:
URL: https://github.com/apache/pinot/pull/15203#discussion_r2140631690
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/ServerInstance.java:
##########
@@ -173,4 +189,20 @@ public int hashCode() {
public String toString() {
return _instanceId;
}
+
+ @VisibleForTesting
+ int extractPool(InstanceConfig instanceConfig) {
+ Map<String, String> pools =
instanceConfig.getRecord().getMapField(InstanceUtils.POOL_KEY);
+ if (pools == null || pools.isEmpty()) {
+ return FALLBACK_REPLICA_GROUP_ID;
+ }
+ Set<String> groups = new HashSet<>(pools.values());
+ if (groups.size() != 1) {
+ LOGGER.warn("Instance: {} belongs to multiple groups: {}", _instanceId,
groups);
+ return FALLBACK_REPLICA_GROUP_ID;
+ }
Review Comment:
Isn't this a supported / expected scenario? I believe a server can belong to
multiple tenants and different instance pools within each tenant tag?
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/ServerInstance.java:
##########
@@ -124,6 +136,10 @@ public int getNettyTlsPort() {
return _nettyTlsPort;
}
+ public int getPool() {
+ return _pool;
+ }
Review Comment:
This is the server's instance pool ID but we're then using it as "replica
group ID" in the instance selector and broker request handler.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -758,6 +761,11 @@ protected BrokerResponse doHandleRequest(long requestId,
String query, SqlNodeAn
remainingTimeMs, serverStats, requestContext);
}
brokerResponse.setTablesQueried(Set.of(rawTableName));
+ brokerResponse.setReplicaGroups(Stream.concat(
+ offlineExecutionServers != null ? offlineExecutionServers.stream()
: Stream.empty(),
+ realtimeExecutionServers != null ?
realtimeExecutionServers.stream() : Stream.empty())
+ .map(ServerInstance::getPool)
+ .collect(Collectors.toSet()));
Review Comment:
Why are we using the server's pool configuration as the replica group that
is being exposed to users (both via the query log as well as through the new
`orderedPreferredReplicas` option)? That doesn't seem right to me - replica
groups can be configured without pool based instance assignment
(https://docs.pinot.apache.org/operators/operating-pinot/instance-assignment#replica-group-instance-assignment).
We already have separate user facing concepts of replica groups and pools and
this seems to mix the two up. The replica group should be determined from the
table's `INSTANCE_PARTITIONS` generated from the instance assignment step and
stored in the ZK property store. Each instance pool can itself have one or more
replica groups assigned to it per table, see
https://github.com/apache/pinot/blob/b5b803e4546a5fc379bee2bbc3f840415511f131/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceReplicaGroupPartitionSelector.java#L154-L162
cc - @Jackie-Jiang @somandal
--
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]
