[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7553: Adjust tuner api

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

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



##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/table/TableConfig.java
##########
@@ -51,7 +51,7 @@
   public static final String UPSERT_CONFIG_KEY = "upsertConfig";
   public static final String INGESTION_CONFIG_KEY = "ingestionConfig";
   public static final String TIER_CONFIGS_LIST_KEY = "tierConfigs";
-  public static final String TUNER_CONFIG = "tunerConfig";
+  public static final String TUNER_CONFIGS_LIST_KEY = "tunerConfigs";

Review comment:
       ```suggestion
     public static final String TUNER_CONFIG_LIST_KEY = "tunerConfigs";
   ```

##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/tuner/TableConfigTunerUtils.java
##########
@@ -18,26 +18,51 @@
  */
 package org.apache.pinot.controller.tuner;
 
+import java.util.List;
+import org.apache.commons.collections.CollectionUtils;
 import org.apache.pinot.controller.helix.core.PinotHelixResourceManager;
 import org.apache.pinot.spi.config.table.TableConfig;
 import org.apache.pinot.spi.config.table.TunerConfig;
 import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 
 public class TableConfigTunerUtils {
+  private static final Logger LOGGER = LoggerFactory.getLogger(TableConfigTunerUtils.class);
+
   private TableConfigTunerUtils() {
   }
 
   /**
-   * Apply TunerConfig to the tableConfig
+   * Apply the entire tunerConfig list inside the tableConfig.
+   */
+  public static void applyTunerConfigs(PinotHelixResourceManager pinotHelixResourceManager, TableConfig tableConfig,
+      Schema schema) {
+    List<TunerConfig> tunerConfigs = tableConfig.getTunerConfigsList();
+    if (CollectionUtils.isNotEmpty(tunerConfigs)) {
+      for (TunerConfig tunerConfig : tunerConfigs) {
+        applyTunerConfig(pinotHelixResourceManager, tunerConfig, tableConfig, schema);
+      }
+    }
+  }
+
+  /**
+   * Apply one explicit tunerConfig to the tableConfig
    */
-  public static void applyTunerConfig(PinotHelixResourceManager pinotHelixResourceManager, TableConfig tableConfig,
+  public static void applyTunerConfig(
+      PinotHelixResourceManager pinotHelixResourceManager, TunerConfig tunerConfig, TableConfig tableConfig,
       Schema schema) {
-    TunerConfig tunerConfig = tableConfig.getTunerConfig();
     if (tunerConfig != null && tunerConfig.getName() != null && !tunerConfig.getName().isEmpty()) {
-      TableConfigTuner tuner = TableConfigTunerRegistry.getTuner(tunerConfig.getName());
-      tuner.init(pinotHelixResourceManager, tunerConfig, schema);
-      tuner.apply(tableConfig);
+      try {
+        TableConfigTuner tuner = TableConfigTunerRegistry.getTuner(tunerConfig.getName());
+        tuner.init(pinotHelixResourceManager, tunerConfig, schema);
+        tuner.apply(tableConfig);
+      } catch (IllegalStateException ise) {
+        LOGGER.error(String.format("Unable to find tuner with name %s", tunerConfig.getName()), ise);

Review comment:
       Suggest merging this exception into the general exception. Based on the exception type we won't know whether the problem is tuner name not registered.

##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/table/TableConfig.java
##########
@@ -95,7 +95,7 @@
   private List<TierConfig> _tierConfigsList;
 
   @JsonPropertyDescription(value = "Configs for Table config tuner")
-  private TunerConfig _tunerConfig;
+  private List<TunerConfig> _tunerConfigs;

Review comment:
       (nit) name it `_tunerConfigList`?




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