narendly commented on a change in pull request #973:
URL: https://github.com/apache/helix/pull/973#discussion_r416953743
##########
File path: helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java
##########
@@ -506,6 +508,16 @@ public void setMaxConcurrentTask(int maxConcurrentTask) {
_record.setIntField(InstanceConfigProperty.MAX_CONCURRENT_TASK.name(),
maxConcurrentTask);
}
+ /**
+ * Get the target size of task thread pool
+ * @return the target size of task thread pool
+ */
+ public int getTargetTaskThreadPoolSize() {
Review comment:
Could we add setters as well with some sort of validation logic?
##########
File path:
helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -102,4 +107,40 @@ public boolean isShutdown() {
public boolean isTerminated() {
return _taskExecutor.isTerminated();
}
+
+ /*
+ * Get target thread pool size from InstanceConfig first; if that fails, get
it from
+ * ClusterConfig; if that fails, fall back to the default value.
+ */
+ private static int _getTaskThreadPoolSize(HelixManager manager) {
Review comment:
Why are we including an underscore for a method name?
##########
File path:
helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -44,11 +49,11 @@
private final ScheduledExecutorService _taskExecutor;
private final ScheduledExecutorService _timerTaskExecutor;
private ThreadPoolExecutorMonitor _monitor;
- public final static int TASK_THREADPOOL_SIZE = 40;
+ public final static int DEFAULT_TASK_THREAD_POOL_SIZE = 40;
Review comment:
Could we potentially move this constant to TaskConstants? Do you think
that would be a good idea?
##########
File path:
helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -102,4 +107,40 @@ public boolean isShutdown() {
public boolean isTerminated() {
return _taskExecutor.isTerminated();
}
+
+ /*
+ * Get target thread pool size from InstanceConfig first; if that fails, get
it from
+ * ClusterConfig; if that fails, fall back to the default value.
+ */
+ private static int _getTaskThreadPoolSize(HelixManager manager) {
+ ConfigAccessor configAccessor = manager.getConfigAccessor();
+ // Check instance config first for thread pool size
+ try {
+ InstanceConfig instanceConfig =
+ configAccessor.getInstanceConfig(manager.getClusterName(),
manager.getInstanceName());
+ int targetTaskThreadPoolSize =
instanceConfig.getTargetTaskThreadPoolSize();
+ if (_verifyTargetThreadPoolSize(targetTaskThreadPoolSize)) {
+ return targetTaskThreadPoolSize;
+ }
+ } catch (HelixException e) {
+ // Pass if InstanceConfig doesn't exist.
+ }
Review comment:
Try-catch is not necessary here. There are other ways to check if
InstanceConfig exists or not.
E.g.) Perform one read, if it's null, then it will return null. If exists,
then you'll have read the config.
##########
File path:
helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -102,4 +107,40 @@ public boolean isShutdown() {
public boolean isTerminated() {
return _taskExecutor.isTerminated();
}
+
+ /*
+ * Get target thread pool size from InstanceConfig first; if that fails, get
it from
+ * ClusterConfig; if that fails, fall back to the default value.
+ */
+ private static int _getTaskThreadPoolSize(HelixManager manager) {
+ ConfigAccessor configAccessor = manager.getConfigAccessor();
+ // Check instance config first for thread pool size
+ try {
+ InstanceConfig instanceConfig =
+ configAccessor.getInstanceConfig(manager.getClusterName(),
manager.getInstanceName());
+ int targetTaskThreadPoolSize =
instanceConfig.getTargetTaskThreadPoolSize();
+ if (_verifyTargetThreadPoolSize(targetTaskThreadPoolSize)) {
+ return targetTaskThreadPoolSize;
+ }
+ } catch (HelixException e) {
+ // Pass if InstanceConfig doesn't exist.
+ }
+
+ // Fallback to cluster config since instance config doesn't provide the
value
+ try {
+ ClusterConfig clusterConfig =
configAccessor.getClusterConfig(manager.getClusterName());
+ int targetTaskThreadPoolSize =
clusterConfig.getTargetTaskThreadPoolSize();
+ if (_verifyTargetThreadPoolSize(targetTaskThreadPoolSize)) {
+ return targetTaskThreadPoolSize;
+ }
+ } catch (HelixException e) {
+ // Pass if ClusterConfig doesn't exist.
+ }
Review comment:
Again, this might be an incorrect use of a try-catch clause
##########
File path:
helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -102,4 +107,40 @@ public boolean isShutdown() {
public boolean isTerminated() {
return _taskExecutor.isTerminated();
}
+
+ /*
+ * Get target thread pool size from InstanceConfig first; if that fails, get
it from
+ * ClusterConfig; if that fails, fall back to the default value.
+ */
+ private static int _getTaskThreadPoolSize(HelixManager manager) {
+ ConfigAccessor configAccessor = manager.getConfigAccessor();
+ // Check instance config first for thread pool size
+ try {
+ InstanceConfig instanceConfig =
+ configAccessor.getInstanceConfig(manager.getClusterName(),
manager.getInstanceName());
+ int targetTaskThreadPoolSize =
instanceConfig.getTargetTaskThreadPoolSize();
+ if (_verifyTargetThreadPoolSize(targetTaskThreadPoolSize)) {
+ return targetTaskThreadPoolSize;
+ }
+ } catch (HelixException e) {
+ // Pass if InstanceConfig doesn't exist.
+ }
+
+ // Fallback to cluster config since instance config doesn't provide the
value
+ try {
+ ClusterConfig clusterConfig =
configAccessor.getClusterConfig(manager.getClusterName());
+ int targetTaskThreadPoolSize =
clusterConfig.getTargetTaskThreadPoolSize();
+ if (_verifyTargetThreadPoolSize(targetTaskThreadPoolSize)) {
+ return targetTaskThreadPoolSize;
+ }
+ } catch (HelixException e) {
+ // Pass if ClusterConfig doesn't exist.
+ }
+
+ return DEFAULT_TASK_THREAD_POOL_SIZE;
+ }
+
+ private static boolean _verifyTargetThreadPoolSize(int
targetTaskThreadPoolSize) {
Review comment:
You could do this in the setter?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
