You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@helix.apache.org by lei-xia <gi...@git.apache.org> on 2015/07/17 19:31:56 UTC
[GitHub] helix pull request: [Helix-606] Add an option in IdealState to all...
GitHub user lei-xia opened a pull request:
https://github.com/apache/helix/pull/32
[Helix-606] Add an option in IdealState to allow a resource to disable showing external view.
Add an option in IdealState to allow a resource to choose not showing external view. This will add flexibility for some resources that the client or application does not care about its external view (such as scheduled jobs), and reduces the ZK traffic when there are a large number of external view listeners.
Add a new configure option in JobConfig to allow use to disable external view when job is scheduling.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lei-xia/helix helix-0.6.x
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/helix/pull/32.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 #32
----
commit 93049be3bf4aadcf3e1345133f9e13daa23cc2d1
Author: Lei Xia <lx...@linkedin.com>
Date: 2015-07-14T01:03:33Z
[Helix-606] Add an option in IdealState to allow a resource to disable showing external view.
Add an option in IdealState to allow a resource to choose not showing external view. This will add flexibility for some resources that the client or application does not care about its external view (such as scheduled jobs), and reduces the ZK traffic when there are a large number of external view listeners.
----
---
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: [Helix-606] Add an option in IdealState to all...
Posted by lei-xia <gi...@git.apache.org>.
Github user lei-xia commented on the pull request:
https://github.com/apache/helix/pull/32#issuecomment-125834788
Merged with head and solved the conflicts. Thanks!
---
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: [Helix-606] Add an option in IdealState to all...
Posted by kanakb <gi...@git.apache.org>.
Github user kanakb commented on the pull request:
https://github.com/apache/helix/pull/32#issuecomment-125834976
LGTM
---
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: [Helix-606] Add an option in IdealState to all...
Posted by kanakb <gi...@git.apache.org>.
Github user kanakb commented on the pull request:
https://github.com/apache/helix/pull/32#issuecomment-125824325
Could you fix the merge conflicts?
---
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: [Helix-606] Add an option in IdealState to all...
Posted by kishoreg <gi...@git.apache.org>.
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/32#discussion_r34959249
--- Diff: helix-core/src/main/java/org/apache/helix/controller/stages/ExternalViewComputeStage.java ---
@@ -78,6 +78,18 @@ public void process(ClusterEvent event) throws Exception {
dataAccessor.getChildValuesMap(keyBuilder.externalViews());
for (String resourceName : resourceMap.keySet()) {
+ IdealState idealState = cache._idealStateMap.get(resourceName);
+
+ // skip generate external view if the ExternalViewDisabled flag is set in IdealState
+ if (idealState != null && idealState.isExternalViewDisabled()) {
--- End diff --
What happens when the resource gets deleted? The isExternalViewDisabled in Idealstate is not available and we might write external views for all the resources.
---
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: [Helix-606] Add an option in IdealState to all...
Posted by lei-xia <gi...@git.apache.org>.
Github user lei-xia commented on the pull request:
https://github.com/apache/helix/pull/32#issuecomment-125794936
Fixed both issues. Thanks!
---
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: [Helix-606] Add an option in IdealState to all...
Posted by kishoreg <gi...@git.apache.org>.
Github user kishoreg commented on the pull request:
https://github.com/apache/helix/pull/32#issuecomment-122674486
So ClusterStateVerifier wont work for such resources? It might be better to enhance the verifier to look at current state if externalview creation is disabled.
Also, there is monitoring mbean which constantly compares idealstate and externalview and emits the differences between the two. With this change, we might see lot of false alerts.
How about real error, what if the task fails how will SRE's know about the ERROR?
Overall code looks good but I am worried that this is deviating quite a bit from Helix concepts.
I understand that this is a short term hack, do you have any ideas on long term fix?
---
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: [Helix-606] Add an option in IdealState to all...
Posted by kishoreg <gi...@git.apache.org>.
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/32#discussion_r35688508
--- Diff: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java ---
@@ -645,7 +645,7 @@ public boolean isLeader() {
if (_helixPropertyStore == null) {
String path = PropertyPathConfig.getPath(PropertyType.PROPERTYSTORE, _clusterName);
- String fallbackPath = String.format("/%s/%s", _clusterName, "HELIX_PROPERTYSTORE");
--- End diff --
This is kept for backward compatibility, we need this when users upgrade from 0.6.2 to newer versions
---
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: [Helix-606] Add an option in IdealState to all...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/helix/pull/32
---
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: [Helix-606] Add an option in IdealState to all...
Posted by kishoreg <gi...@git.apache.org>.
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/32#discussion_r35688395
--- Diff: helix-core/src/main/java/org/apache/helix/controller/stages/ExternalViewComputeStage.java ---
@@ -131,20 +133,30 @@ public void process(ClusterEvent event) throws Exception {
// compare the new external view with current one, set only on different
if (curExtView == null || !curExtView.getRecord().equals(view.getRecord())) {
- keys.add(keyBuilder.externalView(resourceName));
- newExtViews.add(view);
+
+ // For the resource with DisableExternalView option turned on in IdealState
+ // We still compute externalView in above code for verifying and reporting cluster/resource status purpose.
+ // But we will not actually create or write the externalView to ZooKeeper.
+ if (idealState != null && idealState.isExternalViewDisabled()) {
--- End diff --
This logic will not work when idealstate is deleted. Basically idealstate will be null and in the else path we end up creating the external view. This is opposite of what we want.
---
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: [Helix-606] Add an option in IdealState to all...
Posted by lei-xia <gi...@git.apache.org>.
Github user lei-xia commented on the pull request:
https://github.com/apache/helix/pull/32#issuecomment-124138753
For clusterStateVerifier, we will ignore the resources with DisableExternalView set true. Since for now, only job resource will use that option, and usually there are other ways to report error for jobs, instead of comparing IS and EV. (for example, from job context in property store).
For the clusterStatus monitoring bean, that is a good point. I have updated the code in ExternViewComputingStage. Now, instead of totally skipping this stage for such resource as my previous code, we still compute externalView for verifying and reporting cluster/resource status purpose, but we will not actually create or write the externalView to ZooKeeper. This fix the clusterStatus (resource status) mbean problem (verified locally).
Thanks for the review!
---
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.
---