krishan1390 commented on code in PR #15239:
URL: https://github.com/apache/pinot/pull/15239#discussion_r1990594703


##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -1881,22 +1881,47 @@ void validateTableTenantConfig(TableConfig tableConfig) 
{
     }
   }
 
+  /**
+   * Validates if a minion instance is configured for each task type in the 
table config.
+   * This is useful to verify when a new table (or task) is created to not 
miss out on the task execution.
+   * It is also useful when a task's minion instance type or schedule is 
updated.
+   * <p>
+   * However, given minion instances are designed to be scaled down when not 
needed,
+   * we don't require the validation when any other part of the table config 
is updated.
+   */
   @VisibleForTesting
-  void validateTableTaskMinionInstanceTagConfig(TableConfig tableConfig) {
+  void validateTableTaskMinionInstanceTagConfig(TableConfig newTableConfig) {
+
+    TableConfig currentTableConfig = 
getTableConfig(newTableConfig.getTableName());
 
     List<InstanceConfig> allMinionWorkerInstanceConfigs = 
getAllMinionInstanceConfigs();
 
     //extract all minionInstanceTags from allMinionWorkerInstanceConfigs
     Set<String> minionInstanceTagSet = 
allMinionWorkerInstanceConfigs.stream().map(InstanceConfig::getTags)
         .collect(HashSet::new, Set::addAll, Set::addAll);
 
-    if (tableConfig.getTaskConfig() != null && 
tableConfig.getTaskConfig().getTaskTypeConfigsMap() != null) {
-      tableConfig.getTaskConfig().getTaskTypeConfigsMap().forEach((taskType, 
taskTypeConfig) -> {
+    if (newTableConfig.getTaskConfig() != null && 
newTableConfig.getTaskConfig().getTaskTypeConfigsMap() != null) {
+      
newTableConfig.getTaskConfig().getTaskTypeConfigsMap().forEach((taskType, 
taskTypeConfig) -> {
+
         String taskInstanceTag = 
taskTypeConfig.getOrDefault(PinotTaskManager.MINION_INSTANCE_TAG_CONFIG,
             CommonConstants.Helix.UNTAGGED_MINION_INSTANCE);
+        String schedule = 
taskTypeConfig.getOrDefault(PinotTaskManager.SCHEDULE_KEY, "");
+
+        if (currentTableConfig != null && currentTableConfig.getTaskConfig() 
!= null
+            && currentTableConfig.getTaskConfig().isTaskTypeEnabled(taskType)) 
{
+          Map<String, String> currentTaskConfig = 
currentTableConfig.getTaskConfig().getConfigsForTaskType(taskType);
+          String currentTaskInstanceTag = 
currentTaskConfig.getOrDefault(PinotTaskManager.MINION_INSTANCE_TAG_CONFIG,
+              CommonConstants.Helix.UNTAGGED_MINION_INSTANCE);
+          String currentSchedule = 
currentTaskConfig.getOrDefault(PinotTaskManager.SCHEDULE_KEY, "");
+          if (currentTaskInstanceTag.equals(taskInstanceTag) && 
currentSchedule.equals(schedule)) {

Review Comment:
   Done. 



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

Reply via email to