gortiz commented on code in PR #13816:
URL: https://github.com/apache/pinot/pull/13816#discussion_r1716386801
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/utils/BrokerSelectorUtils.java:
##########
@@ -57,11 +59,13 @@ public static List<String>
getTablesCommonBrokers(List<String> tableNames, Map<S
return null;
}
- List<String> commonBrokers = tablesBrokersList.get(0);
- for (int i = 1; i < tablesBrokersList.size(); i++) {
- commonBrokers.retainAll(tablesBrokersList.get(i));
- }
- return commonBrokers;
+ // Make a copy of the brokersList for the first table. retainAll does
inplace modifications corrupting brokerData.
+ Set<String> commonBrokers = tablesBrokersList.stream()
+ .skip(1) // skip because the HashSet is initialized with the first
list.
+ .collect(() -> new HashSet<>(tablesBrokersList.get(0)),
Set::retainAll, Set::retainAll);
+ List<String> commonBrokerList = new ArrayList<>(commonBrokers.size());
+ commonBrokerList.addAll(commonBrokers);
+ return commonBrokerList;
Review Comment:
I found this quite more difficult to read (and probably more expensive in
allocation) than the original. Why not just something like:
```
List<String> commonBrokers = new ArrayList<>(tablesBrokersList.get(0));
for (int i = 1; i < tablesBrokersList.size(); i++) {
commonBrokers.retainAll(tablesBrokersList.get(i));
}
return commonBrokers;
```
Alternatively commonBrokers can be changed to a set if you want to optimize
that computation (at the cost of an extra allocation)
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
