vrajat commented on code in PR #13361:
URL: https://github.com/apache/pinot/pull/13361#discussion_r1696324162
##########
pinot-common/src/main/java/org/apache/pinot/common/broker/BrokerSelectorUtils.java:
##########
@@ -36,8 +35,9 @@ private BrokerSelectorUtils() {
* @param brokerData: map holding data for table hosting on brokers.
* @return list of common brokers hosting all the tables.
*/
- public static List<String> getTablesCommonBrokers(List<String> tableNames,
Map<String, List<String>> brokerData) {
- List<List<String>> tablesBrokersList = new ArrayList<>();
+ public static List<BrokerInfo> getTablesCommonBrokers(List<String>
tableNames,
+ Map<String, List<BrokerInfo>> brokerData) {
+ List<List<BrokerInfo>> tablesBrokersList = new ArrayList<>();
Review Comment:
There were two ways to get the common tenants. One way is what you
suggested. `PinotQueryResource` implements this option.
The other method is to get the mapping of table->brokers mapping and find
the common brokers. The mapping is read from `BROKER_EXTERNAL_VIEW_PATH` The
mapping also takes into consideration liveness of the broker. This method is
used in `DynamicBrokerSelector`.
I discussed this with @Jackie-Jiang and he suggested to use the
table->brokers mapping as that is the source of truth. So `brokerData` is
populated from this mapping and refreshed whenever there is a change in the
liveness of brokers.
--
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
