Copilot commented on code in PR #19166:
URL: https://github.com/apache/pinot/pull/19166#discussion_r3733559058
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/MultiClusterRoutingManager.java:
##########
@@ -217,8 +217,37 @@ public List<String> getSegments(BrokerRequest
brokerRequest, @Nullable String sa
return combined.isEmpty() ? null : combined;
}
+ /// Returns the partition info only when a single cluster has any, and
`null` when more than one does.
+ ///
+ /// Unlike [#getRoutingTable], [#getSegments] and [#getServingInstances],
this cannot union the clusters: the info is
+ /// a per-partition array of the servers holding every segment of that
partition, and no server holds the segments
+ /// that live in another cluster. One cluster's array would make a partition
served only by another cluster look like
+ /// a partition holding no data, and a colocated join treats such a
partition as empty and silently drops its rows. So
+ /// a table spread over several clusters reports nothing and its callers
fail. Expressing it properly needs the array
+ /// to carry each partition's cluster, which the current shape cannot do.
@Override
public TablePartitionReplicatedServersInfo
getTablePartitionReplicatedServersInfo(String tableNameWithType) {
- return findFirst(mgr ->
mgr.getTablePartitionReplicatedServersInfo(tableNameWithType),
tableNameWithType);
+ TablePartitionReplicatedServersInfo partitionInfo =
+
_localClusterRoutingManager.getTablePartitionReplicatedServersInfo(tableNameWithType);
Review Comment:
The local lookup has the same ambiguity as a remote `null`: the table may
have a routing entry but no usable partition metadata. If a remote cluster
later returns metadata, this method would return that remote-only array for a
table also served locally. Detect local routing here and return `null` before
accepting another cluster's partial view.
This issue also appears in the following locations of the same file:
- line 236
- line 241
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/WorkerManager.java:
##########
@@ -1242,8 +1615,17 @@ private PartitionTableInfo
calculatePartitionTableInfo(String tableName) {
partitionInfoMap[i] = new PartitionInfo(fullyReplicatedServers,
offlinePartitionInfo._segments,
realtimePartitionInfo._segments);
}
+ // Union the two sides, then keep only the partitions the merged map
has no entry for: a partition one side
+ // deferred but the other still serves as a whole does get a worker,
so reporting it would fail a query the
+ // other side can answer on its own. A TreeSet keeps the broker's
sorted order, so the error message is
+ // deterministic.
+ Set<Integer> partitionsWithOnlyDeferredSegments =
+ new TreeSet<>(offlineTpi.getPartitionsWithOnlyDeferredSegments());
+
partitionsWithOnlyDeferredSegments.addAll(realtimeTpi.getPartitionsWithOnlyDeferredSegments());
+ partitionsWithOnlyDeferredSegments.removeIf(
+ partitionId -> partitionId < partitionInfoMap.length &&
partitionInfoMap[partitionId] != null);
Review Comment:
A deferred partition on either hybrid side still contains rows that the
other side cannot replace. Removing it merely because the merged map has a
REALTIME/OFFLINE entry lets planning proceed with only that entry, silently
omitting the deferred side's rows. Keep the union of deferred partitions so
partition-aware planning fails until every data-bearing side is servable.
--
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]
