Jackie-Jiang commented on code in PR #14159:
URL: https://github.com/apache/pinot/pull/14159#discussion_r1805536556
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -69,6 +69,8 @@ public class ControllerConf extends PinotConfiguration {
public static final String CONTROLLER_MODE = "controller.mode";
public static final String LEAD_CONTROLLER_RESOURCE_REBALANCE_STRATEGY =
"controller.resource.rebalance.strategy";
public static final String LEAD_CONTROLLER_RESOURCE_REBALANCE_DELAY_MS =
"controller.resource.rebalance.delay_ms";
+ private static final String LEAD_CONTROLLER_RESOURCE_REBALANCE_NUM_THREADS =
Review Comment:
Let's keep them `public` in case we want to use these constants to set config
##########
pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java:
##########
@@ -266,6 +266,16 @@ public void init(PinotConfiguration pinotConfiguration)
TableConfigUtils.setEnforcePoolBasedAssignment(_config.isEnforcePoolBasedAssignmentEnabled());
}
+ // If thread pool size is not configured executor will use cached thread pool
+ private ExecutorService createExecutorService(int numThreadPool, String
threadNameFormat) {
+ ThreadFactory threadFactory = new ThreadFactoryBuilder()
+ .setNameFormat(threadNameFormat)
+ .build();
+ return (numThreadPool == 0)
Review Comment:
For robustness
```suggestion
return (numThreadPool <= 0)
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -69,6 +69,8 @@ public class ControllerConf extends PinotConfiguration {
public static final String CONTROLLER_MODE = "controller.mode";
public static final String LEAD_CONTROLLER_RESOURCE_REBALANCE_STRATEGY =
"controller.resource.rebalance.strategy";
public static final String LEAD_CONTROLLER_RESOURCE_REBALANCE_DELAY_MS =
"controller.resource.rebalance.delay_ms";
+ private static final String LEAD_CONTROLLER_RESOURCE_REBALANCE_NUM_THREADS =
Review Comment:
This config is not for lead controller resource (lead controller resource is
a Helix resource to manage lead controller). Let's rename it to
`controller.executor.rebalance.numThreads`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -408,6 +412,10 @@ public void setRealtimeSegmentCommitTimeoutSeconds(int
timeoutSec) {
setProperty(SEGMENT_COMMIT_TIMEOUT_SECONDS, Integer.toString(timeoutSec));
}
+ public void setControllerNumThreadPool(int numThreadPool) {
Review Comment:
Make the method name align with config, e.g.
`setControllerExecutorNumThreads()`, same for other places
##########
pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java:
##########
@@ -266,6 +266,16 @@ public void init(PinotConfiguration pinotConfiguration)
TableConfigUtils.setEnforcePoolBasedAssignment(_config.isEnforcePoolBasedAssignmentEnabled());
}
+ // If thread pool size is not configured executor will use cached thread pool
+ private ExecutorService createExecutorService(int numThreadPool, String
threadNameFormat) {
+ ThreadFactory threadFactory = new ThreadFactoryBuilder()
Review Comment:
(format) Re-format with Pinot Style
--
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
