kfaraz commented on code in PR #12592:
URL: https://github.com/apache/druid/pull/12592#discussion_r892067534
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/ForkingTaskRunner.java:
##########
@@ -635,7 +646,7 @@ public Optional<ScalingStats> getScalingStats()
@Override
public void start()
Review Comment:
Probably needs to be annotated with `@LifecycleStart`. I wonder if just
annotating the interface method `TaskRunner.start()` would work so that the
implementations don't need to worry about this.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/ForkingTaskRunner.java:
##########
@@ -214,6 +218,13 @@ public TaskStatus call()
command.add("-cp");
command.add(taskClasspath);
+ if (numProcessorsPerTask < 1) {
Review Comment:
Nit: Is this really needed? Won't `TaskRunner.run()` always be called after
the lifecycle start method?
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/ForkingTaskRunnerTest.java:
##########
@@ -373,6 +375,7 @@ int waitForTaskProcessToComplete(Task task, ProcessHolder
processHolder, File lo
}
};
+ forkingTaskRunner.setNumProcessorsPerTask();
Review Comment:
Could we call `forkingTaskRunner.start()` here instead to avoid exposing the
`setNumProcessorsPerTask()` as a `@VisibleForTesting` method?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
