[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6468: Adding cluster config to config number of concurrent tasks per instance for minion task: SegmentGenerationAndPushTaskGenerator

[GitBox <gi...@apache.org>]

Jackie-Jiang commented on a change in pull request #6468:
URL: https://github.com/apache/incubator-pinot/pull/6468#discussion_r562119547



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/ClusterInfoAccessor.java
##########
@@ -157,4 +160,20 @@ public void setRealtimeToOfflineSegmentsTaskMetadata(
   public String getVipUrl() {
     return _controllerConf.generateVipUrl();
   }
+
+  /**
+   * Get the cluster config for a given config name, return null if not found.
+   *
+   * @return cluster config
+   */
+  public String getClusterConfig(String configName) {
+    HelixConfigScope helixConfigScope = new HelixConfigScopeBuilder(HelixConfigScope.ConfigScopeProperty.CLUSTER)
+        .forCluster(_pinotHelixResourceManager.getHelixClusterName()).build();
+    Map<String, String> configMap =
+        _pinotHelixResourceManager.getHelixAdmin().getConfig(helixConfigScope, Arrays.asList(configName));

Review comment:
       (nit)
   ```suggestion
           _pinotHelixResourceManager.getHelixAdmin().getConfig(helixConfigScope, Collections.singletonList(configName));
   ```

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/common/MinionConstants.java
##########
@@ -92,6 +92,8 @@ private MinionConstants() {
   // Generate segment and push to controller based on batch ingestion configs
   public static class SegmentGenerationAndPushTask {
     public static final String TASK_TYPE = "SegmentGenerationAndPushTask";
+    public static final String CONFIG_NUMBER_CONCURRENT_TASKS_PER_INSTANCE =
+        "pinot.minion.task.generator.SegmentGenerationAndPushTask.numConcurrentTasksPerInstance";

Review comment:
       Simplify this key to `SegmentGenerationAndPushTask.numConcurrentTasksPerInstance`? The current one looks like an instance config key

##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/ClusterInfoAccessor.java
##########
@@ -157,4 +160,20 @@ public void setRealtimeToOfflineSegmentsTaskMetadata(
   public String getVipUrl() {
     return _controllerConf.generateVipUrl();
   }
+
+  /**
+   * Get the cluster config for a given config name, return null if not found.
+   *
+   * @return cluster config
+   */
+  public String getClusterConfig(String configName) {
+    HelixConfigScope helixConfigScope = new HelixConfigScopeBuilder(HelixConfigScope.ConfigScopeProperty.CLUSTER)
+        .forCluster(_pinotHelixResourceManager.getHelixClusterName()).build();
+    Map<String, String> configMap =
+        _pinotHelixResourceManager.getHelixAdmin().getConfig(helixConfigScope, Arrays.asList(configName));
+    if (configMap == null || !configMap.containsKey(configName)) {
+      return null;
+    }
+    return configMap.get(configName);

Review comment:
       (nit) Can be simplified to
   ```suggestion
       return configMap != null ? configMap.get(configName) : null;
   ```




----------------------------------------------------------------
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.

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