You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@helix.apache.org by dasahcc <gi...@git.apache.org> on 2017/05/10 19:22:46 UTC
[GitHub] helix pull request #91: [Helix-656] Support customize batch state transition...
GitHub user dasahcc opened a pull request:
https://github.com/apache/helix/pull/91
[Helix-656] Support customize batch state transition thread pool
To better support batch message handling, we shall make batch state transition thread pool configurable. This config can be put in cluster config.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dasahcc/helix helix-0.6.x
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/helix/pull/91.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #91
----
commit 6a78f7de0cdb298a6dccbf9802eb63204e983211
Author: Junkai Xue <jx...@linkedin.com>
Date: 2017-05-10T19:18:41Z
[Helix-656] Support customize batch state transition thread pool
To better support batch message handling, we shall make batch state transition thread pool configurable.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request #91: [Helix-656] Support customize batch state transition...
Posted by dasahcc <gi...@git.apache.org>.
Github user dasahcc commented on a diff in the pull request:
https://github.com/apache/helix/pull/91#discussion_r115865002
--- Diff: helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java ---
@@ -210,6 +213,18 @@ private void updateStateTransitionMessageThreadPool(Message message, HelixManage
return;
}
+ if (!_batchMessageThreadpoolChecked) {
--- End diff --
There is no HelixManger or ZkClient to read configurable information from ZK in constructor.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request #91: [Helix-656] Support customize batch state transition...
Posted by kongweihan <gi...@git.apache.org>.
Github user kongweihan commented on a diff in the pull request:
https://github.com/apache/helix/pull/91#discussion_r115862430
--- Diff: helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java ---
@@ -210,6 +213,18 @@ private void updateStateTransitionMessageThreadPool(Message message, HelixManage
return;
}
+ if (!_batchMessageThreadpoolChecked) {
--- End diff --
It seems to me as long as there's any `STATE_TRANSITION` message, this pool will be created, so why not simply instantiate this thread pool in the constructor?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request #91: [Helix-656] Support customize batch state transition...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/helix/pull/91
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request #91: [Helix-656] Support customize batch state transition...
Posted by dasahcc <gi...@git.apache.org>.
Github user dasahcc commented on a diff in the pull request:
https://github.com/apache/helix/pull/91#discussion_r115886683
--- Diff: helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java ---
@@ -210,6 +213,18 @@ private void updateStateTransitionMessageThreadPool(Message message, HelixManage
return;
}
+ if (!_batchMessageThreadpoolChecked) {
--- End diff --
This is not a proper solution for the problem. We shall keep the backward compatible. HelixTaskExecutor has a default constructor now. In this case, if the HelixTaskExecutor instantiated by default constructor does not have any information for configurable threadpool. That's why there is also a logic to make the threadpool configurable in a separate method (not only for batch message thread pool). Otherwise, the user defined the information will be ignored. It is not good just remove the public constructor.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request #91: [Helix-656] Support customize batch state transition...
Posted by kongweihan <gi...@git.apache.org>.
Github user kongweihan commented on a diff in the pull request:
https://github.com/apache/helix/pull/91#discussion_r115901354
--- Diff: helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java ---
@@ -210,6 +213,18 @@ private void updateStateTransitionMessageThreadPool(Message message, HelixManage
return;
}
+ if (!_batchMessageThreadpoolChecked) {
--- End diff --
I see, just to confirm, is backward compatibility the only concern here?
I'm wondering if constructing HelixTaskExecutor with a HelixManager would make more sense, at all, if we're free to change the constructor?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request #91: [Helix-656] Support customize batch state transition...
Posted by kongweihan <gi...@git.apache.org>.
Github user kongweihan commented on a diff in the pull request:
https://github.com/apache/helix/pull/91#discussion_r115871919
--- Diff: helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java ---
@@ -210,6 +213,18 @@ private void updateStateTransitionMessageThreadPool(Message message, HelixManage
return;
}
+ if (!_batchMessageThreadpoolChecked) {
--- End diff --
Would it be better to pass it in? Each HelixManager for sure has a DefaultMessagingService, which for sure has a HelixTaskExecutor. Using NotificationContext to pass in the "parent object" just to do initialization doesn't seem quite straightforward.
When I work on adding ThreadPoolExecutorMonitor for HelixTaskExecutor, I actually have the same problem of getting InstanceType and ClusterName from HelixManager. Maybe we should add it in the constructor?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---