You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/05/05 22:41:42 UTC
[GitHub] [helix] narendly commented on a change in pull request #973: TaskStateModelFactory configurable thread pool size
narendly commented on a change in pull request #973:
URL: https://github.com/apache/helix/pull/973#discussion_r420428755
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -137,6 +141,7 @@
private final static int MAX_REBALANCE_PREFERENCE = 10;
private final static int MIN_REBALANCE_PREFERENCE = 0;
public final static boolean DEFAULT_GLOBAL_REBALANCE_ASYNC_MODE_ENABLED = true;
+ public static final int DEFAULT_TARGET_TASK_THREAD_POOL_SIZE_NOT_SET = -1;
Review comment:
I find myself repeating this again, why does this field need to be public?
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -709,6 +714,32 @@ public void setInstanceCapacityKeys(List<String> capacityKeys) {
_record.setListField(ClusterConfigProperty.INSTANCE_CAPACITY_KEYS.name(), capacityKeys);
}
+ /**
+ * Get the default target size of task thread pools. This values applies to participants and is
+ * overwritten by participants' own values if they specified individual pool sizes in
+ * InstanceConfigs
+ * @return the target size of task thread pool
+ */
+ public int getDefaultTargetTaskThreadPoolSize() {
+ return _record.getIntField(
+ ClusterConfig.ClusterConfigProperty.DEFAULT_TARGET_TASK_THREAD_POOL_SIZE.name(),
Review comment:
We're confusing terminology here. Let's please try to be exact:
What you want here is the global target task thread pool size, not default.
The "default" value refers to 40. It's okay to use the NOT_SET bit and resort to 40 at initialization time.
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -709,6 +714,32 @@ public void setInstanceCapacityKeys(List<String> capacityKeys) {
_record.setListField(ClusterConfigProperty.INSTANCE_CAPACITY_KEYS.name(), capacityKeys);
}
+ /**
+ * Get the default target size of task thread pools. This values applies to participants and is
+ * overwritten by participants' own values if they specified individual pool sizes in
+ * InstanceConfigs
+ * @return the target size of task thread pool
+ */
+ public int getDefaultTargetTaskThreadPoolSize() {
+ return _record.getIntField(
+ ClusterConfig.ClusterConfigProperty.DEFAULT_TARGET_TASK_THREAD_POOL_SIZE.name(),
+ DEFAULT_TARGET_TASK_THREAD_POOL_SIZE_NOT_SET);
+ }
+
+ /**
+ * Set the default target size of task thread pools for this cluster.
+ * @param defaultTargetTaskThreadPoolSize - the new target task thread pool size
+ * @throws IllegalArgumentException - when the provided new thread pool size is not greater than 0
+ */
+ public void setDefaultTargetTaskThreadPoolSize(int defaultTargetTaskThreadPoolSize)
Review comment:
Fix the name.
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -109,7 +109,11 @@
// https://github.com/apache/helix/wiki/Weight-aware-Globally-Evenly-distributed-Rebalancer#rebalance-coordinator
//
// Default to be true.
- GLOBAL_REBALANCE_ASYNC_MODE
+ GLOBAL_REBALANCE_ASYNC_MODE,
+
+ // The target size of task thread pools for each participant. This is the "default" value
Review comment:
"global" value to the cluster
##########
File path: helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java
##########
@@ -56,11 +56,13 @@
DOMAIN,
DELAY_REBALANCE_ENABLED,
MAX_CONCURRENT_TASK,
- INSTANCE_CAPACITY_MAP
+ INSTANCE_CAPACITY_MAP,
+ TARGET_TASK_THREAD_POOL_SIZE
}
public static final int WEIGHT_NOT_SET = -1;
public static final int MAX_CONCURRENT_TASK_NOT_SET = -1;
+ public static final int TARGET_TASK_THREAD_POOL_SIZE_NOT_SET = -1;
Review comment:
private?
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -709,6 +714,32 @@ public void setInstanceCapacityKeys(List<String> capacityKeys) {
_record.setListField(ClusterConfigProperty.INSTANCE_CAPACITY_KEYS.name(), capacityKeys);
}
+ /**
+ * Get the default target size of task thread pools. This values applies to participants and is
+ * overwritten by participants' own values if they specified individual pool sizes in
+ * InstanceConfigs
+ * @return the target size of task thread pool
+ */
+ public int getDefaultTargetTaskThreadPoolSize() {
+ return _record.getIntField(
+ ClusterConfig.ClusterConfigProperty.DEFAULT_TARGET_TASK_THREAD_POOL_SIZE.name(),
+ DEFAULT_TARGET_TASK_THREAD_POOL_SIZE_NOT_SET);
+ }
+
+ /**
+ * Set the default target size of task thread pools for this cluster.
+ * @param defaultTargetTaskThreadPoolSize - the new target task thread pool size
+ * @throws IllegalArgumentException - when the provided new thread pool size is not greater than 0
+ */
+ public void setDefaultTargetTaskThreadPoolSize(int defaultTargetTaskThreadPoolSize)
+ throws IllegalArgumentException {
+ if (defaultTargetTaskThreadPoolSize <= 0) {
+ throw new IllegalArgumentException("targetTaskThreadPoolSize must be greater than 0!");
Review comment:
Fix the message.
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -709,6 +714,32 @@ public void setInstanceCapacityKeys(List<String> capacityKeys) {
_record.setListField(ClusterConfigProperty.INSTANCE_CAPACITY_KEYS.name(), capacityKeys);
}
+ /**
+ * Get the default target size of task thread pools. This values applies to participants and is
+ * overwritten by participants' own values if they specified individual pool sizes in
+ * InstanceConfigs
+ * @return the target size of task thread pool
+ */
+ public int getDefaultTargetTaskThreadPoolSize() {
+ return _record.getIntField(
+ ClusterConfig.ClusterConfigProperty.DEFAULT_TARGET_TASK_THREAD_POOL_SIZE.name(),
+ DEFAULT_TARGET_TASK_THREAD_POOL_SIZE_NOT_SET);
+ }
+
+ /**
+ * Set the default target size of task thread pools for this cluster.
Review comment:
Not default, but global value. The default value for the global config should be 40.
##########
File path: helix-core/src/main/java/org/apache/helix/model/LiveInstance.java
##########
@@ -190,6 +192,24 @@ public void setWebserviceUrl(String url) {
_record.setSimpleField(LiveInstanceProperty.ZKPROPERTYTRANSFERURL.toString(), url);
}
+ /**
+ * Get the current task thread pool size of the instance
+ * @return the current task thread pool size
+ */
+ public int getCurrentTaskThreadPoolSize() {
+ return _record.getIntField(LiveInstanceProperty.CURRENT_TASK_THREAD_POOL_SIZE.name(),
+ CURRENT_TASK_THREAD_POOL_SIZE_NOT_SET);
Review comment:
Please try to understand the proposed design well. This suggests to me that you should perhaps spend more time trying to understand what this field represents.
For example, what does it mean for an instance to have a task thread pool of size -1? (it is meaningless, we shouldn't create a task pool with size -1).
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -109,7 +109,11 @@
// https://github.com/apache/helix/wiki/Weight-aware-Globally-Evenly-distributed-Rebalancer#rebalance-coordinator
//
// Default to be true.
- GLOBAL_REBALANCE_ASYNC_MODE
+ GLOBAL_REBALANCE_ASYNC_MODE,
+
+ // The target size of task thread pools for each participant. This is the "default" value
+ //that's used when participants don't specify their individual pool sizes.
Review comment:
Nit: fix the comment (space)
##########
File path: helix-core/src/main/java/org/apache/helix/model/LiveInstance.java
##########
@@ -48,6 +49,7 @@
TASK_EXEC_THREAD
}
+ public static final int CURRENT_TASK_THREAD_POOL_SIZE_NOT_SET = -1;
Review comment:
This field is not necessary.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org