Re: [PR] Consistency in API response for live brokers [pinot]

["Jackie-Jiang (via GitHub)" <gi...@apache.org>]

Jackie-Jiang commented on code in PR #11850:
URL: https://github.com/apache/pinot/pull/11850#discussion_r1397855016


##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3923,18 +3935,21 @@ public Map<String, List<InstanceInfo>> getTableToLiveBrokersMapping() {
         .collect(Collectors.toMap(InstanceConfig::getInstanceName, Function.identity()));
 
     Map<String, List<InstanceInfo>> result = new HashMap<>();
+    if (optionalTableName.isPresent()) {
+      String tableName = optionalTableName.get();
+      List<InstanceInfo> hosts = getBrokersFromTableNameWithType(ev, tableName, instanceConfigMap,
+              this::getLiveBrokerInstancesForTable);
+      if (!hosts.isEmpty()) {
+        result.put(tableName, hosts);

Review Comment:
   I think we want to put hosts when table exists, even if it is empty



##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3908,11 +3908,23 @@ public TableStats getTableStats(String tableNameWithType) {
     return new TableStats(creationTime);
   }
 
+  @FunctionalInterface
+  interface Func<T> {
+    List<T> apply(ExternalView ev, String tableName, Map<String, String> brokersToState,
+            Map<String, InstanceConfig> instanceConfigMap);
+  }
+
+  static <T> List<T> getBrokers(ExternalView ev, String tableName, Map<String, String> brokersToState,
+          Map<String, InstanceConfig> instanceConfigMap, Func<T> relevantBrokerFunc) {
+    return relevantBrokerFunc.apply(ev, tableName, brokersToState, instanceConfigMap);
+  }

Review Comment:
   These are not common functions, and I find them quite hard to read. Can we keep it simple? It is okay if we need to repeat some code



##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3923,18 +3935,21 @@ public Map<String, List<InstanceInfo>> getTableToLiveBrokersMapping() {
         .collect(Collectors.toMap(InstanceConfig::getInstanceName, Function.identity()));
 
     Map<String, List<InstanceInfo>> result = new HashMap<>();
+    if (optionalTableName.isPresent()) {
+      String tableName = optionalTableName.get();

Review Comment:
   This `tableName` can be raw (without type suffix). The returned map should contain the actual table name with type suffix



##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java:
##########
@@ -165,9 +166,11 @@ public List<String> getLiveBrokersForTable(
   @ApiResponses(value = {
       @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 500, message = "Internal server error")
   })
-  public Map<String, List<InstanceInfo>> getLiveBrokers() {
+  public Map<String, List<InstanceInfo>> getLiveBrokers(
+          @ApiParam(value = "Table name (with or without type)", required = false)
+          @QueryParam("tablename") Optional<String> tablename) {

Review Comment:
   Query param should be `tableName`
   Shall we keep it consistent with other APIs where optional field is represented as `@Nullable`
   ```suggestion
             @QueryParam("tableName") @Nullable tableName) {
   ```



-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org