[GitHub] [pinot] mcvsubbu commented on a diff in pull request #8465: Add adhoc minion task creation endpoint

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

mcvsubbu commented on code in PR #8465:
URL: https://github.com/apache/pinot/pull/8465#discussion_r843277312


##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTaskRestletResource.java:
##########
@@ -346,6 +347,29 @@ public StringResultResponse getTaskStateDeprecated(
     }
   }
 
+  @GET
+  @Path("/tasks/all")
+  @Authenticate(AccessType.READ)
+  @ApiOperation("Read all tasks status")
+  public Map<String, TaskState> getAllTasks() {
+    Map<String, TaskState> taskStates = new HashMap<>();
+    _pinotHelixTaskResourceManager.getTaskTypes()
+        .forEach(taskType -> taskStates.putAll(_pinotHelixTaskResourceManager.getTaskStates(taskType)));
+    return taskStates;
+  }
+
+  @POST
+  @Path("/tasks/create")
+  @Authenticate(AccessType.CREATE)

Review Comment:
   Instead of taking a generic map, can we add specific arguments here? The arguments can be self-documented instead of the user learning one argument at a time based on the exception thrown.
   
   Or, maybe this can be a table API (since we always want to run tasks on a particular table)



##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTaskRestletResource.java:
##########
@@ -346,6 +347,29 @@ public StringResultResponse getTaskStateDeprecated(
     }
   }
 
+  @GET
+  @Path("/tasks/all")
+  @Authenticate(AccessType.READ)
+  @ApiOperation("Read all tasks status")
+  public Map<String, TaskState> getAllTasks() {
+    Map<String, TaskState> taskStates = new HashMap<>();
+    _pinotHelixTaskResourceManager.getTaskTypes()
+        .forEach(taskType -> taskStates.putAll(_pinotHelixTaskResourceManager.getTaskStates(taskType)));
+    return taskStates;
+  }
+
+  @POST
+  @Path("/tasks/create")

Review Comment:
   I think what you want to do is to {{run}} a specific task type that has already been defined. Correct? If so, can we change the API to call it "run" (or "execute")? 



##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTaskRestletResource.java:
##########
@@ -346,6 +347,29 @@ public StringResultResponse getTaskStateDeprecated(
     }
   }
 
+  @GET
+  @Path("/tasks/all")

Review Comment:
   Can we do that in the client? (i.e. fetch all tasks and then iterate through the types to get status)?



##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java:
##########
@@ -128,6 +132,53 @@ public PinotTaskManager(PinotHelixTaskResourceManager helixTaskResourceManager,
     }
   }
 
+  public String createTask(Map<String, String> taskConfigs)
+      throws Exception {
+    String taskName = taskConfigs.get("taskName");
+    if (taskName == null) {
+      throw new IllegalArgumentException("Missing field 'taskName'");
+    }
+    String taskType = taskConfigs.get("taskType");
+    if (taskType == null) {
+      throw new IllegalArgumentException("Missing field 'taskType'");
+    }
+    String tableName = taskConfigs.get("tableName");
+    if (tableName == null) {
+      throw new IllegalArgumentException("Missing field 'tableName'");
+    }
+    String parentTaskName = _helixTaskResourceManager.getParentTaskName(taskType, taskName);
+    TaskState taskState = _helixTaskResourceManager.getTaskState(parentTaskName);
+    if (taskState != null) {
+      throw new RuntimeException(
+          "Task [" + taskName + "] of type [" + taskType + "] is already created. Current state is " + taskState);
+    }
+    if (TableNameBuilder.isRealtimeTableResource(tableName)) {

Review Comment:
   Why this limitation ? Let the task decide if it is able to work on realtime or not



##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/FileIngestionHelper.java:
##########
@@ -176,7 +176,7 @@ public static void copyURIToLocal(Map<String, String> batchConfigMap, URI source
     if (!PinotFSFactory.isSchemeSupported(sourceFileURIScheme)) {
       PinotFSFactory.register(sourceFileURIScheme, batchConfigMap.get(BatchConfigProperties.INPUT_FS_CLASS),
           IngestionConfigUtils.getInputFsProps(IngestionConfigUtils.getConfigMapWithPrefix(
-              batchConfigMap, BatchConfigProperties.INPUT_FS_PROP_PREFIX)));
+              batchConfigMap, BatchConfigProperties.INPUT_FS_PROP_PREFIX + IngestionConfigUtils.DOT_SEPARATOR)));

Review Comment:
   How is this related to adding an adhoc task api?  It is better to keep bug fixes independent.



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