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/03/04 00:40:21 UTC
[GitHub] [helix] zhangmeng916 opened a new pull request #851: Customized
view controller
zhangmeng916 opened a new pull request #851: Customized view controller
URL: https://github.com/apache/helix/pull/851
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
(#850 )
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
Helix will support customized state input from its users and the aggregation of customized state to customized view. This PR implement the computation logic for customized view aggregation and also modify Helix generic controller to include an extra stage to compute customized view from customized state.
### Tests
- [X] The following tests are written for this issue:
TestCustomizedStateComputationStage
TestCustomizedViewStage
TestComputeAndCleanupCustomizedView
- [X] The following is the result of the "mvn test" command on the appropriate module:
[INFO] Tests run: 909, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3,438.097 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 909, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
### Commits
- [X] My commits all reference appropriate Apache Helix GitHub issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- [X] My diff has been formatted using helix-style.xml
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on issue #851: Modify Helix generic
controller to include new stage for customized view aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on issue #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#issuecomment-604083600
This PR is ready to merge, approved by @jiajunwang
Final commit message:
Add new stages in Helix generic controller for customized view aggregation.
- Add extra stages and pipelines in controller for customized state computation and customized view aggregation.
- Add refresh logic in resource data provider for customized view related data refresh.
- Add customized state event handling in CallbackHandler.
- Add integration test for customized view aggregation.
- Modify existing tests to verify new logic.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r398143820
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -822,6 +822,11 @@ public void onCustomizedStateRootChange(String instanceName, List<String> custom
logger.info("START: GenericClusterController.onCustomizedStateRootChange()");
HelixManager manager = changeContext.getManager();
Builder keyBuilder = new Builder(manager.getClusterName());
+ if (customizedStateTypes.isEmpty()) {
+ customizedStateTypes =
+ manager.getHelixDataAccessor().getChildNames(
+ manager.getHelixDataAccessor().keyBuilder().customizedStatesRoot(instanceName));
Review comment:
nit, you have the keyBuilder above.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r388613231
##########
File path: helix-core/src/main/java/org/apache/helix/PropertyPathBuilder.java
##########
@@ -268,6 +271,18 @@ public static String externalView(String clusterName, String resourceName) {
return String.format("/%s/EXTERNALVIEW/%s", clusterName, resourceName);
}
+ public static String customizedView(String clusterName) {
+ return String.format("/%s/CUSTOMIZEDVIEW", clusterName);
+ }
+
+ public static String customizedView(String clusterName, String customizedStateName) {
+ return String.format("/%s/EXTERNALVIEW/%s", clusterName, customizedStateName);
+ }
+
+ public static String customizedView(String clusterName, String customizedStateName, String resourceName) {
+ return String.format("/%s/EXTERNALVIEW/%s/%s", clusterName, customizedStateName, resourceName);
Review comment:
Same here.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r396796706
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -775,6 +811,39 @@ public void onStateChange(String instanceName, List<CurrentState> statesInfo,
logger.info("END: GenericClusterController.onStateChange()");
}
+ @Override
+ @PreFetch(enabled = false)
+ public void onCustomizedStateRootChange(String instanceName, NotificationContext changeContext) {
+ logger.info("START: GenericClusterController.onCustomizedStateRootChange()");
+ notifyCaches(changeContext, ChangeType.CUSTOMIZED_STATE_ROOT);
+ HelixManager manager = changeContext.getManager();
+ List<String> customizedStateTypes =
+ manager.getHelixDataAccessor().getChildNames(
+ manager.getHelixDataAccessor().keyBuilder().customizedStatesRoot(instanceName));
+
+ for (String customizedState : customizedStateTypes) {
Review comment:
Shall we do the deletion here as well?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r395884785
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -569,12 +570,19 @@ public void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
});
}
+ @Override
+ public void addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
+ String instanceName) throws Exception {
+ addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
+ ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ }
+
@Override
public void addCustomizedStateChangeListener(CustomizedStateChangeListener listener,
- String instanceName, String customizedStateName) throws Exception {
- addListener(listener, new Builder(_clusterName).customizedStates(instanceName, customizedStateName),
- ChangeType.CUSTOMIZED_STATE, new EventType[] { EventType.NodeChildrenChanged
- });
+ String instanceName, String customizedStateType) throws Exception {
Review comment:
Yes, because under customized state, there're different types of customized states, and their children are different resources, and then customized state data. Only listen to one level of children is not enough.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r388621712
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1072,6 +1152,25 @@ protected void checkLiveInstancesObservation(List<LiveInstance> liveInstances,
}
}
+ for (String instance : curInstances.keySet()) {
+ if (lastInstances == null || !lastInstances.containsKey(instance)) {
+ try {
Review comment:
And I guess this method could be referred in the method onCustomizedStateAggregationConfigChange() above.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r396866020
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -259,45 +266,52 @@ private void parseListenerProperties() {
Class listenerClass = null;
switch (_changeType) {
- case IDEAL_STATE:
- listenerClass = IdealStateChangeListener.class;
- break;
- case INSTANCE_CONFIG:
- if (_listener instanceof ConfigChangeListener) {
+ case IDEAL_STATE:
+ listenerClass = IdealStateChangeListener.class;
+ break;
+ case INSTANCE_CONFIG:
+ if (_listener instanceof ConfigChangeListener) {
+ listenerClass = ConfigChangeListener.class;
+ } else if (_listener instanceof InstanceConfigChangeListener) {
+ listenerClass = InstanceConfigChangeListener.class;
+ }
+ break;
+ case CLUSTER_CONFIG:
+ listenerClass = ClusterConfigChangeListener.class;
+ break;
+ case RESOURCE_CONFIG:
+ listenerClass = ResourceConfigChangeListener.class;
+ break;
+ case CUSTOMIZED_STATE_CONFIG:
+ listenerClass = CustomizedStateConfigChangeListener.class;
+ break;
+ case CONFIG:
listenerClass = ConfigChangeListener.class;
- } else if (_listener instanceof InstanceConfigChangeListener) {
- listenerClass = InstanceConfigChangeListener.class;
- }
- break;
- case CLUSTER_CONFIG:
- listenerClass = ClusterConfigChangeListener.class;
- break;
- case RESOURCE_CONFIG:
- listenerClass = ResourceConfigChangeListener.class;
- break;
- case CONFIG:
- listenerClass = ConfigChangeListener.class;
- break;
- case LIVE_INSTANCE:
- listenerClass = LiveInstanceChangeListener.class;
- break;
- case CURRENT_STATE:
- listenerClass = CurrentStateChangeListener.class;
- ;
- break;
- case MESSAGE:
- case MESSAGES_CONTROLLER:
- listenerClass = MessageListener.class;
- break;
- case EXTERNAL_VIEW:
- case TARGET_EXTERNAL_VIEW:
- listenerClass = ExternalViewChangeListener.class;
- break;
- case CUSTOMIZED_VIEW:
- listenerClass = CustomizedViewChangeListener.class;
- break;
- case CONTROLLER:
- listenerClass = ControllerChangeListener.class;
+ break;
+ case LIVE_INSTANCE:
+ listenerClass = LiveInstanceChangeListener.class;
+ break;
+ case CURRENT_STATE:
+ listenerClass = CurrentStateChangeListener.class;
+ break;
+ case CUSTOMIZED_STATE_ROOT:
+ listenerClass = CustomizedStateRootChangeListener.class;
Review comment:
I would prefer to keep them separate for clarity, and later when we get rid of the middle layer, we can delete the redundant one.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r397408106
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -450,8 +451,10 @@ public void invoke(NotificationContext changeContext) throws Exception {
CustomizedStateRootChangeListener customizedStateRootChangeListener =
(CustomizedStateRootChangeListener) _listener;
String instanceName = PropertyPathConfig.getInstanceNameFromPath(_path);
- customizedStateRootChangeListener.onCustomizedStateRootChange(instanceName,
- changeContext);
+ List<String> customizedStateTypes =
Review comment:
If prefetch enabled, then we read. Otherwise, we should pass an empty list.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r394683530
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -899,6 +941,38 @@ public void onResourceConfigChange(
.info("END: GenericClusterController.onResourceConfigChange() for cluster " + _clusterName);
}
+ @Override
+ @PreFetch(enabled = false)
+ public void onCustomizedStateConfigChange(
+ CustomizedStateConfig customizedStateConfig,
+ NotificationContext context) {
+ HelixManager helixManager = context.getManager();
Review comment:
I remember we agreed on moving this to the event processing logic. So there won't be a major delay in the ZK event callback thread.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r388642767
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
##########
@@ -96,13 +99,15 @@
// Special caches
private CurrentStateCache _currentStateCache;
+ private CustomizedStateCache _customizedStateCache;
Review comment:
We aggregate _currentStateCache in addition to externalView because it is used in the rebalance pipeline. For customize state though, the customized view is the only thing we need. Can we just combine these 2 refresh workflow and keep one customized View map in the cache?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r397409147
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -813,23 +817,36 @@ public void onStateChange(String instanceName, List<CurrentState> statesInfo,
@Override
@PreFetch(enabled = false)
- public void onCustomizedStateRootChange(String instanceName, NotificationContext changeContext) {
+ public void onCustomizedStateRootChange(String instanceName, List<String> customizedStateTypes,
+ NotificationContext changeContext) {
logger.info("START: GenericClusterController.onCustomizedStateRootChange()");
- notifyCaches(changeContext, ChangeType.CUSTOMIZED_STATE_ROOT);
HelixManager manager = changeContext.getManager();
- List<String> customizedStateTypes =
- manager.getHelixDataAccessor().getChildNames(
- manager.getHelixDataAccessor().keyBuilder().customizedStatesRoot(instanceName));
+ Builder keyBuilder = new Builder(manager.getClusterName());
- for (String customizedState : customizedStateTypes) {
- try {
- manager.addCustomizedStateChangeListener(this, instanceName, customizedState);
- logger.info(
- manager.getInstanceName() + " added customized state listener for " + instanceName
- + ", listener: " + this);
- } catch (Exception e) {
- logger.error("Fail to add customized state listener for instance: " + instanceName, e);
+ synchronized (_lastSeenCustomizedStateTypes) {
+ Set<String> lastSeenCustomizedStateTypes = _lastSeenCustomizedStateTypes.get();
Review comment:
If we already lock on _lastSeenCustomizedStateTypes, do we still need to use AtomicReference?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r388613940
##########
File path: helix-core/src/main/java/org/apache/helix/PropertyPathBuilder.java
##########
@@ -373,6 +388,10 @@ public static String resourceConfig(String clusterName) {
return String.format("/%s/CONFIGS/RESOURCE", clusterName);
}
+ public static String customizedStateAggregationConfig(String clusterName) {
+ return String.format("/%s/CONFIGS/CUSTOMIZED_STATE_AGGREGATION", clusterName);
Review comment:
I think it is possible that we have other configs for the customized state except for AGGREGATION. It would be easier for us to extend if we just call it CUSTOMIZED_STATE, which means all CUSTOMIZED_STATE configuration.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851:
Add customized view computation and modify Helix generic controller to
include new stage
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r390026861
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
##########
@@ -96,13 +99,15 @@
// Special caches
private CurrentStateCache _currentStateCache;
+ private CustomizedStateCache _customizedStateCache;
Review comment:
So _customizedStateCache is used by CustomizedStateComputation stage, which happened before customized view aggregation stage. Therefore, we do need this cache to avoid access to ZK during computation.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r394564092
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
##########
@@ -96,13 +99,15 @@
// Special caches
private CurrentStateCache _currentStateCache;
+ private CustomizedStateCache _customizedStateCache;
Review comment:
Have moved it to resource provider.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r395290320
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -743,6 +743,8 @@ private void createZKPaths(String clusterName) {
_zkClient.createPersistent(path);
path = PropertyPathBuilder.resourceConfig(clusterName);
_zkClient.createPersistent(path);
+ path = PropertyPathBuilder.customizedStateConfig(clusterName);
Review comment:
added in TestZkHelixAdmin
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r394041314
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -122,7 +124,7 @@
// TODO: make this be per _manager or per _listener instaed of per callbackHandler -- Lei
private CallbackProcessor _batchCallbackProcessor;
private boolean _watchChild = true; // Whether we should subscribe to the child znode's data
- // change.
+ // change.
Review comment:
Maybe no need to format the whole file? Or you can move this comment before the code:
// comment
code;
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on issue #851: Modify Helix generic
controller to include new stage for customized view aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on issue #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#issuecomment-600905403
One more thing, the added test cases are not enough. Please add tests for the following features:
1. We need to add a case in the test listener leakage for the new listener introduced.
2. Admin change to create the new paths.
3. In the integration test, please test adjusting the view config and see if the view changed accordingly. Moreover, we should use the customized view provider to test.
4. Try to cover some error case, for example, instances are not reporting the configured type. Or 2 instances are reporting different sets of types.
Please try to cover all the use cases that we mentioned in the design doc.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r394035326
##########
File path: helix-core/src/test/java/org/apache/helix/integration/TestComputeAndCleanupCustomizedView.java
##########
@@ -0,0 +1,166 @@
+package org.apache.helix.integration;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.ArrayList;
+import java.util.Date;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.TestHelper;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.ZkTestHelper;
+import org.apache.helix.ZkUnitTestBase;
+import org.apache.helix.integration.manager.ClusterControllerManager;
+import org.apache.helix.integration.manager.MockParticipantManager;
+import org.apache.helix.manager.zk.ZKHelixAdmin;
+import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.apache.helix.model.CustomizedState;
+import org.apache.helix.model.CustomizedStateConfig;
+import org.apache.helix.model.CustomizedView;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import static java.lang.Thread.*;
+
+
+/**
+ * Test compute and clean customized view - if customized state is remove externally, controller should remove the
+ * orphan customized view
+ */
+public class TestComputeAndCleanupCustomizedView extends ZkUnitTestBase {
+
+ private final String RESOURCE_NAME = "TestDB0";
+ private final String PARTITION_NAME = "TestDB0_0";
+ private final String CUSTOMIZED_STATE_NAME = "customizedState1";
+ private final String INSTANCE_NAME1 = "localhost_12918";
+ private final String INSTANCE_NAME2 = "localhost_12919";
+
+ @Test
+ public void test() throws Exception {
+ String className = TestHelper.getTestClassName();
+ String methodName = TestHelper.getTestMethodName();
+ String clusterName = className + "_" + methodName;
+ int n = 2;
+
+ System.out.println("START " + clusterName + " at " + new Date(System.currentTimeMillis()));
+
+ TestHelper.setupCluster(clusterName, ZK_ADDR, 12918, // participant port
+ "localhost", // participant name prefix
+ "TestDB", // resource name prefix
+ 1, // resources
+ 2, // partitions per resource
+ n, // number of nodes
+ 2, // replicas
+ "MasterSlave", true); // do rebalance
+
+ ClusterControllerManager controller =
+ new ClusterControllerManager(ZK_ADDR, clusterName, "controller_0");
+ controller.syncStart();
+
+ // start participants
+ MockParticipantManager[] participants = new MockParticipantManager[n];
+ for (int i = 0; i < n; i++) {
+ String instanceName = "localhost_" + (12918 + i);
+
+ participants[i] = new MockParticipantManager(ZK_ADDR, clusterName, instanceName);
+ participants[i].syncStart();
+ }
+
+ ZKHelixDataAccessor accessor =
+ new ZKHelixDataAccessor(clusterName, new ZkBaseDataAccessor<ZNRecord>(_gZkClient));
+ PropertyKey.Builder keyBuilder = accessor.keyBuilder();
+
+ CustomizedStateConfig config = new CustomizedStateConfig();
+ List<String> aggregationEnabledTypes = new ArrayList<>();
+ aggregationEnabledTypes.add(CUSTOMIZED_STATE_NAME);
+ config.setAggregationEnabledTypes(aggregationEnabledTypes);
+
+ accessor.setProperty(keyBuilder.customizedStateConfig(), config);
+
+ CustomizedState customizedState = new CustomizedState(RESOURCE_NAME);
+ customizedState.setState(PARTITION_NAME, "STARTED");
+ accessor.setProperty(
+ keyBuilder.customizedState(INSTANCE_NAME1, CUSTOMIZED_STATE_NAME, RESOURCE_NAME),
+ customizedState);
+
+ CustomizedView customizedView = null;
+ int i = 0;
+ while (true) {
+ sleep(1000);
Review comment:
It seems this is a case that you can use `TestHelper.verify()`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r394036061
##########
File path: helix-core/src/test/java/org/apache/helix/integration/TestComputeAndCleanupCustomizedView.java
##########
@@ -0,0 +1,166 @@
+package org.apache.helix.integration;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.ArrayList;
+import java.util.Date;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.TestHelper;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.ZkTestHelper;
+import org.apache.helix.ZkUnitTestBase;
+import org.apache.helix.integration.manager.ClusterControllerManager;
+import org.apache.helix.integration.manager.MockParticipantManager;
+import org.apache.helix.manager.zk.ZKHelixAdmin;
+import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.apache.helix.model.CustomizedState;
+import org.apache.helix.model.CustomizedStateConfig;
+import org.apache.helix.model.CustomizedView;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import static java.lang.Thread.*;
+
+
+/**
+ * Test compute and clean customized view - if customized state is remove externally, controller should remove the
+ * orphan customized view
+ */
+public class TestComputeAndCleanupCustomizedView extends ZkUnitTestBase {
+
+ private final String RESOURCE_NAME = "TestDB0";
+ private final String PARTITION_NAME = "TestDB0_0";
+ private final String CUSTOMIZED_STATE_NAME = "customizedState1";
+ private final String INSTANCE_NAME1 = "localhost_12918";
+ private final String INSTANCE_NAME2 = "localhost_12919";
+
+ @Test
+ public void test() throws Exception {
+ String className = TestHelper.getTestClassName();
+ String methodName = TestHelper.getTestMethodName();
+ String clusterName = className + "_" + methodName;
+ int n = 2;
+
+ System.out.println("START " + clusterName + " at " + new Date(System.currentTimeMillis()));
+
+ TestHelper.setupCluster(clusterName, ZK_ADDR, 12918, // participant port
+ "localhost", // participant name prefix
+ "TestDB", // resource name prefix
+ 1, // resources
+ 2, // partitions per resource
+ n, // number of nodes
+ 2, // replicas
+ "MasterSlave", true); // do rebalance
+
+ ClusterControllerManager controller =
+ new ClusterControllerManager(ZK_ADDR, clusterName, "controller_0");
+ controller.syncStart();
+
+ // start participants
+ MockParticipantManager[] participants = new MockParticipantManager[n];
+ for (int i = 0; i < n; i++) {
+ String instanceName = "localhost_" + (12918 + i);
+
+ participants[i] = new MockParticipantManager(ZK_ADDR, clusterName, instanceName);
+ participants[i].syncStart();
+ }
+
+ ZKHelixDataAccessor accessor =
+ new ZKHelixDataAccessor(clusterName, new ZkBaseDataAccessor<ZNRecord>(_gZkClient));
+ PropertyKey.Builder keyBuilder = accessor.keyBuilder();
+
+ CustomizedStateConfig config = new CustomizedStateConfig();
+ List<String> aggregationEnabledTypes = new ArrayList<>();
+ aggregationEnabledTypes.add(CUSTOMIZED_STATE_NAME);
+ config.setAggregationEnabledTypes(aggregationEnabledTypes);
+
+ accessor.setProperty(keyBuilder.customizedStateConfig(), config);
+
+ CustomizedState customizedState = new CustomizedState(RESOURCE_NAME);
+ customizedState.setState(PARTITION_NAME, "STARTED");
+ accessor.setProperty(
+ keyBuilder.customizedState(INSTANCE_NAME1, CUSTOMIZED_STATE_NAME, RESOURCE_NAME),
+ customizedState);
+
+ CustomizedView customizedView = null;
+ int i = 0;
+ while (true) {
+ sleep(1000);
+ try {
+ customizedView = accessor.getProperty(keyBuilder.customizedView(CUSTOMIZED_STATE_NAME, RESOURCE_NAME));
+ Map<String, String> stateMap = customizedView.getRecord().getMapField(PARTITION_NAME);
+ if (stateMap.get(INSTANCE_NAME1).equals("STARTED")) {
+ System.out.println("succeed");
+ break;
+ }
+ } catch (Exception e) {
+ i ++;
+ if (i >= 10) {
+ Assert.fail("The customized view is not correct");
+ }
+ }
+ }
+
+ // disable controller
+ ZKHelixAdmin admin = new ZKHelixAdmin(_gZkClient);
+ admin.enableCluster(clusterName, false);
+ ZkTestHelper.tryWaitZkEventsCleaned(controller.getZkClient());
+
+ // drop resource
+ admin.dropResource(clusterName, RESOURCE_NAME);
+
+ // delete customized state manually, controller shall remove customized view when cluster is enabled again
+
+ accessor.removeProperty(
+ keyBuilder.customizedState(INSTANCE_NAME1, CUSTOMIZED_STATE_NAME, RESOURCE_NAME));
+ accessor.removeProperty(
+ keyBuilder.currentState(INSTANCE_NAME2, CUSTOMIZED_STATE_NAME, RESOURCE_NAME));
+
+ // re-enable controller shall remove orphan external view
+ // System.out.println("re-enabling controller");
+ admin.enableCluster(clusterName, true);
+
+ customizedView = null;
+ for (i = 0; i < 10; i++) {
+ sleep(100);
+ customizedView = accessor.getProperty(keyBuilder.customizedView(CUSTOMIZED_STATE_NAME));
+ if (customizedView == null) {
+ break;
+ }
+ }
Review comment:
`TestHelper.verify()` would help.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #851: Modify
Helix generic controller to include new stage for customized view aggregation
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r395372128
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -569,12 +570,19 @@ public void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
});
}
+ @Override
+ public void addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
+ String instanceName) throws Exception {
+ addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
+ ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ }
+
@Override
public void addCustomizedStateChangeListener(CustomizedStateChangeListener listener,
- String instanceName, String customizedStateName) throws Exception {
- addListener(listener, new Builder(_clusterName).customizedStates(instanceName, customizedStateName),
- ChangeType.CUSTOMIZED_STATE, new EventType[] { EventType.NodeChildrenChanged
- });
+ String instanceName, String customizedStateType) throws Exception {
Review comment:
We subscribe on root of customized state for children change. Do we still need for each indivdual?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r394685581
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -899,6 +941,38 @@ public void onResourceConfigChange(
.info("END: GenericClusterController.onResourceConfigChange() for cluster " + _clusterName);
}
+ @Override
+ @PreFetch(enabled = false)
+ public void onCustomizedStateConfigChange(
+ CustomizedStateConfig customizedStateConfig,
+ NotificationContext context) {
+ HelixManager helixManager = context.getManager();
Review comment:
Another question, when do we remove the listener if the corresponding type has been removed from the config?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r396824725
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -569,12 +570,19 @@ public void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
});
}
+ @Override
+ public void addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
+ String instanceName) throws Exception {
+ addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
+ ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ }
+
@Override
public void addCustomizedStateChangeListener(CustomizedStateChangeListener listener,
- String instanceName, String customizedStateName) throws Exception {
- addListener(listener, new Builder(_clusterName).customizedStates(instanceName, customizedStateName),
- ChangeType.CUSTOMIZED_STATE, new EventType[] { EventType.NodeChildrenChanged
- });
+ String instanceName, String customizedStateType) throws Exception {
Review comment:
That would be one additional layer than the current callbackhandler infrastructure can support.
I would like to have a try to improve it after this PR is in.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r398159643
##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/CustomizedViewAggregateStage.java
##########
@@ -0,0 +1,171 @@
+package org.apache.helix.controller.stages;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.HelixManager;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.common.caches.CustomizedViewCache;
+import org.apache.helix.controller.LogUtil;
+import org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.controller.pipeline.AbstractAsyncBaseStage;
+import org.apache.helix.controller.pipeline.AsyncWorkerType;
+import org.apache.helix.controller.pipeline.StageException;
+import org.apache.helix.model.CustomizedView;
+import org.apache.helix.model.Partition;
+import org.apache.helix.model.Resource;
+import org.apache.helix.monitoring.mbeans.ClusterStatusMonitor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class CustomizedViewAggregateStage extends AbstractAsyncBaseStage {
+ private static Logger LOG = LoggerFactory.getLogger(CustomizedViewAggregateStage.class);
+
+ @Override
+ public AsyncWorkerType getAsyncWorkerType() {
+ return AsyncWorkerType.CustomizedStateViewComputeWorker;
+ }
+
+ @Override
+ public void execute(final ClusterEvent event) throws Exception {
Review comment:
If you take a look at it now, it's simpler than before. Right now it only has remove stale state types, and update customized view logic.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r394690097
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -743,6 +743,8 @@ private void createZKPaths(String clusterName) {
_zkClient.createPersistent(path);
path = PropertyPathBuilder.resourceConfig(clusterName);
_zkClient.createPersistent(path);
+ path = PropertyPathBuilder.customizedStateConfig(clusterName);
Review comment:
Is this change covered by any unit tests?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r396800481
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -259,45 +266,52 @@ private void parseListenerProperties() {
Class listenerClass = null;
switch (_changeType) {
- case IDEAL_STATE:
- listenerClass = IdealStateChangeListener.class;
- break;
- case INSTANCE_CONFIG:
- if (_listener instanceof ConfigChangeListener) {
+ case IDEAL_STATE:
+ listenerClass = IdealStateChangeListener.class;
+ break;
+ case INSTANCE_CONFIG:
+ if (_listener instanceof ConfigChangeListener) {
+ listenerClass = ConfigChangeListener.class;
+ } else if (_listener instanceof InstanceConfigChangeListener) {
+ listenerClass = InstanceConfigChangeListener.class;
+ }
+ break;
+ case CLUSTER_CONFIG:
+ listenerClass = ClusterConfigChangeListener.class;
+ break;
+ case RESOURCE_CONFIG:
+ listenerClass = ResourceConfigChangeListener.class;
+ break;
+ case CUSTOMIZED_STATE_CONFIG:
+ listenerClass = CustomizedStateConfigChangeListener.class;
+ break;
+ case CONFIG:
listenerClass = ConfigChangeListener.class;
- } else if (_listener instanceof InstanceConfigChangeListener) {
- listenerClass = InstanceConfigChangeListener.class;
- }
- break;
- case CLUSTER_CONFIG:
- listenerClass = ClusterConfigChangeListener.class;
- break;
- case RESOURCE_CONFIG:
- listenerClass = ResourceConfigChangeListener.class;
- break;
- case CONFIG:
- listenerClass = ConfigChangeListener.class;
- break;
- case LIVE_INSTANCE:
- listenerClass = LiveInstanceChangeListener.class;
- break;
- case CURRENT_STATE:
- listenerClass = CurrentStateChangeListener.class;
- ;
- break;
- case MESSAGE:
- case MESSAGES_CONTROLLER:
- listenerClass = MessageListener.class;
- break;
- case EXTERNAL_VIEW:
- case TARGET_EXTERNAL_VIEW:
- listenerClass = ExternalViewChangeListener.class;
- break;
- case CUSTOMIZED_VIEW:
- listenerClass = CustomizedViewChangeListener.class;
- break;
- case CONTROLLER:
- listenerClass = ControllerChangeListener.class;
+ break;
+ case LIVE_INSTANCE:
+ listenerClass = LiveInstanceChangeListener.class;
+ break;
+ case CURRENT_STATE:
+ listenerClass = CurrentStateChangeListener.class;
+ break;
+ case CUSTOMIZED_STATE_ROOT:
+ listenerClass = CustomizedStateRootChangeListener.class;
Review comment:
This can also use the same listener class if you move the handling method to CustomizedStateChangeListener.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r394689766
##########
File path: helix-core/src/test/java/org/apache/helix/integration/TestZkCallbackHandlerLeak.java
##########
@@ -123,7 +123,7 @@ public boolean verify() throws Exception {
// printHandlers(participantManagerToExpire);
int controllerHandlerNb = controller.getHandlers().size();
int particHandlerNb = participantManagerToExpire.getHandlers().size();
- Assert.assertEquals(controllerHandlerNb, 11,
+ Assert.assertEquals(controllerHandlerNb, 12,
"HelixController should have 10 (5+2n) callback handlers for 2 (n) participant");
Review comment:
update the comment? Or the math won't be right.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r388631215
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
##########
@@ -96,13 +99,15 @@
// Special caches
private CurrentStateCache _currentStateCache;
+ private CustomizedStateCache _customizedStateCache;
private InstanceMessagesCache _instanceMessagesCache;
// Other miscellaneous caches
private Map<String, Long> _instanceOfflineTimeMap;
private Map<String, Map<String, String>> _idealStateRuleMap;
private Map<String, Map<String, Set<String>>> _disabledInstanceForPartitionMap = new HashMap<>();
private Set<String> _disabledInstanceSet = new HashSet<>();
+ private Set<String> _aggregationEnabledTypes = new HashSet<>();
Review comment:
I see it is updated, but I don't see anyone uses it. So a little bit confused. Could you please specify the plan of using it in this comment?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r397411992
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -813,23 +817,36 @@ public void onStateChange(String instanceName, List<CurrentState> statesInfo,
@Override
@PreFetch(enabled = false)
- public void onCustomizedStateRootChange(String instanceName, NotificationContext changeContext) {
+ public void onCustomizedStateRootChange(String instanceName, List<String> customizedStateTypes,
+ NotificationContext changeContext) {
logger.info("START: GenericClusterController.onCustomizedStateRootChange()");
- notifyCaches(changeContext, ChangeType.CUSTOMIZED_STATE_ROOT);
HelixManager manager = changeContext.getManager();
- List<String> customizedStateTypes =
- manager.getHelixDataAccessor().getChildNames(
- manager.getHelixDataAccessor().keyBuilder().customizedStatesRoot(instanceName));
+ Builder keyBuilder = new Builder(manager.getClusterName());
- for (String customizedState : customizedStateTypes) {
- try {
- manager.addCustomizedStateChangeListener(this, instanceName, customizedState);
- logger.info(
- manager.getInstanceName() + " added customized state listener for " + instanceName
- + ", listener: " + this);
- } catch (Exception e) {
- logger.error("Fail to add customized state listener for instance: " + instanceName, e);
+ synchronized (_lastSeenCustomizedStateTypes) {
+ Set<String> lastSeenCustomizedStateTypes = _lastSeenCustomizedStateTypes.get();
+ for (String customizedState : customizedStateTypes) {
Review comment:
Note that if prefetch is false, you should not expect customizedStateTypes to be filled with data.
The code works here because you are not following the prefetch configuration in the callback handler logic.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r388617004
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -423,6 +431,10 @@ private static PipelineRegistry createDefaultRegistry(String pipelineName) {
Pipeline externalViewPipeline = new Pipeline(pipelineName);
externalViewPipeline.addStage(new ExternalViewComputeStage());
+ // customized state view generation
+ Pipeline customizedViewPipeline = new Pipeline(pipelineName);
+ customizedViewPipeline.addStage(new CustomizedViewComputeStage());
Review comment:
CustomizedViewAggregateStage ?
I just try to differentiate it from CustomizedStateComputationStage.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r396824077
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -424,6 +446,20 @@ public void invoke(NotificationContext changeContext) throws Exception {
List<CurrentState> currentStates = preFetch(_propertyKey);
currentStateChangeListener.onStateChange(instanceName, currentStates, changeContext);
+ } else if (_changeType == CUSTOMIZED_STATE_ROOT) {
+ CustomizedStateRootChangeListener customizedStateRootChangeListener =
+ (CustomizedStateRootChangeListener) _listener;
+ String instanceName = PropertyPathConfig.getInstanceNameFromPath(_path);
+ customizedStateRootChangeListener.onCustomizedStateRootChange(instanceName,
Review comment:
If prefetch, I guess we should get all the children for types and pass to the onCustomizedStateRootChange() method?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r394678586
##########
File path: helix-core/src/main/java/org/apache/helix/common/caches/CustomizedViewCache.java
##########
@@ -154,4 +154,4 @@ public void clear() {
_customizedViewCache.clear();
_customizedViewMap.clear();
}
-}
+}
Review comment:
nit, What's the delta here? Can we avoid unnecessary change in PR?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r394689083
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -743,6 +743,8 @@ private void createZKPaths(String clusterName) {
_zkClient.createPersistent(path);
path = PropertyPathBuilder.resourceConfig(clusterName);
_zkClient.createPersistent(path);
+ path = PropertyPathBuilder.customizedStateConfig(clusterName);
Review comment:
This is OK for this PR, but ideally, we should separate this type of API change into other PRs.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r394661721
##########
File path: helix-core/src/test/java/org/apache/helix/controller/stages/TestCustomizedViewStage.java
##########
@@ -0,0 +1,105 @@
+package org.apache.helix.controller.stages;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixManager;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.TestHelper;
+import org.apache.helix.ZkUnitTestBase;
+import org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.controller.pipeline.Pipeline;
+import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.apache.helix.model.CustomizedState;
+import org.apache.helix.model.CustomizedStateConfig;
+import org.apache.helix.model.CustomizedView;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+public class TestCustomizedViewStage extends ZkUnitTestBase {
+ private final String RESOURCE_NAME = "testResourceName";
+ private final String PARTITION_NAME = "testResourceName_0";
+ private final String CUSTOMIZED_STATE_NAME = "customizedState1";
+ private final String INSTANCE_NAME = "localhost_1";
+
+ @Test
+ public void testCachedCustomizedViews() throws Exception {
+ String clusterName = "CLUSTER_" + TestHelper.getTestMethodName();
+
+ HelixDataAccessor accessor =
+ new ZKHelixDataAccessor(clusterName, new ZkBaseDataAccessor<>(_gZkClient));
+ HelixManager manager = new DummyClusterManager(clusterName, accessor);
+
+ setupLiveInstances(clusterName, new int[]{0, 1});
+ setupStateModel(clusterName);
+
+ ClusterEvent event = new ClusterEvent(ClusterEventType.Unknown);
+ ResourceControllerDataProvider cache = new ResourceControllerDataProvider(clusterName);
+ event.addAttribute(AttributeName.helixmanager.name(), manager);
+ event.addAttribute(AttributeName.ControllerDataProvider.name(), cache);
+
+ CustomizedStateConfig config = new CustomizedStateConfig();
+ List<String> aggregationEnabledTypes = new ArrayList<>();
+ aggregationEnabledTypes.add(CUSTOMIZED_STATE_NAME);
+ config.setAggregationEnabledTypes(aggregationEnabledTypes);
+
+ PropertyKey.Builder keyBuilder = accessor.keyBuilder();
+ accessor.setProperty(keyBuilder.customizedStateConfig(), config);
+
+ CustomizedState customizedState = new CustomizedState(RESOURCE_NAME);
+ customizedState.setState(PARTITION_NAME, "STARTED");
+ accessor
+ .setProperty(keyBuilder.customizedState(INSTANCE_NAME, "customizedState1", RESOURCE_NAME),
+ customizedState);
+
+ CustomizedViewAggregateStage customizedViewComputeStage = new CustomizedViewAggregateStage();
+ Pipeline dataRefresh = new Pipeline();
+ dataRefresh.addStage(new ReadClusterDataStage());
+ runPipeline(event, dataRefresh);
+ runStage(event, new ResourceComputationStage());
+ runStage(event, new CustomizedStateComputationStage());
+ runStage(event, customizedViewComputeStage);
+ Assert.assertEquals(cache.getCustomizedViewCacheMap().values(),
+ accessor.getChildValues(accessor.keyBuilder().customizedViews()));
+
+ // Assure there is no customized view got updated
+ List<CustomizedView> oldCustomizedViews =
+ accessor.getChildValues(accessor.keyBuilder().customizedViews());
+ runStage(event, customizedViewComputeStage);
+ List<CustomizedView> newCustomizedViews =
+ accessor.getChildValues(accessor.keyBuilder().customizedViews());
+ Assert.assertEquals(oldCustomizedViews, newCustomizedViews);
Review comment:
Discussed in another PR, and will keep it as a list for now as we're verifying based on index.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r388614308
##########
File path: helix-core/src/main/java/org/apache/helix/common/caches/CustomizedStateCache.java
##########
@@ -49,11 +49,15 @@ public CustomizedStateCache(ControlContextProvider contextProvider) {
Map<String, LiveInstance> liveInstanceMap) {
Set<PropertyKey> participantStateKeys = new HashSet<>();
PropertyKey.Builder keyBuilder = accessor.keyBuilder();
- Set<String> restrictedKeys = new HashSet<>(
- accessor.getProperty(accessor.keyBuilder().customizedStateAggregationConfig()).getRecord()
- .getListFields().get(
- CustomizedStateAggregationConfig.CustomizedStateAggregationProperty.AGGREGATION_ENABLED_TYPES
- .name()));
+
+ Set<String> restrictedKeys = new HashSet<>();
+ if (accessor.getProperty(accessor.keyBuilder().customizedStateAggregationConfig()) != null) {
Review comment:
Since the getProperty() may cause real ZK traffic. Can we get once and assign it to a local var?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r388621065
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1072,6 +1152,25 @@ protected void checkLiveInstancesObservation(List<LiveInstance> liveInstances,
}
}
+ for (String instance : curInstances.keySet()) {
+ if (lastInstances == null || !lastInstances.containsKey(instance)) {
+ try {
Review comment:
A method to warp the logic would be cleaner.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r394034424
##########
File path: helix-core/src/main/java/org/apache/helix/common/caches/CustomizedStateCache.java
##########
@@ -27,6 +27,7 @@
import org.apache.helix.PropertyKey;
import org.apache.helix.common.controllers.ControlContextProvider;
import org.apache.helix.model.CustomizedState;
+import org.apache.helix.model.CustomizedStateConfig;
Review comment:
It seems this import is unused?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r394034905
##########
File path: helix-core/src/test/java/org/apache/helix/integration/TestComputeAndCleanupCustomizedView.java
##########
@@ -0,0 +1,166 @@
+package org.apache.helix.integration;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.ArrayList;
+import java.util.Date;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.TestHelper;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.ZkTestHelper;
+import org.apache.helix.ZkUnitTestBase;
+import org.apache.helix.integration.manager.ClusterControllerManager;
+import org.apache.helix.integration.manager.MockParticipantManager;
+import org.apache.helix.manager.zk.ZKHelixAdmin;
+import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.apache.helix.model.CustomizedState;
+import org.apache.helix.model.CustomizedStateConfig;
+import org.apache.helix.model.CustomizedView;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import static java.lang.Thread.*;
+
+
+/**
+ * Test compute and clean customized view - if customized state is remove externally, controller should remove the
+ * orphan customized view
+ */
+public class TestComputeAndCleanupCustomizedView extends ZkUnitTestBase {
+
+ private final String RESOURCE_NAME = "TestDB0";
+ private final String PARTITION_NAME = "TestDB0_0";
+ private final String CUSTOMIZED_STATE_NAME = "customizedState1";
+ private final String INSTANCE_NAME1 = "localhost_12918";
+ private final String INSTANCE_NAME2 = "localhost_12919";
+
+ @Test
+ public void test() throws Exception {
+ String className = TestHelper.getTestClassName();
+ String methodName = TestHelper.getTestMethodName();
+ String clusterName = className + "_" + methodName;
+ int n = 2;
+
+ System.out.println("START " + clusterName + " at " + new Date(System.currentTimeMillis()));
+
+ TestHelper.setupCluster(clusterName, ZK_ADDR, 12918, // participant port
+ "localhost", // participant name prefix
+ "TestDB", // resource name prefix
+ 1, // resources
+ 2, // partitions per resource
+ n, // number of nodes
+ 2, // replicas
+ "MasterSlave", true); // do rebalance
+
+ ClusterControllerManager controller =
+ new ClusterControllerManager(ZK_ADDR, clusterName, "controller_0");
+ controller.syncStart();
+
+ // start participants
+ MockParticipantManager[] participants = new MockParticipantManager[n];
+ for (int i = 0; i < n; i++) {
+ String instanceName = "localhost_" + (12918 + i);
+
+ participants[i] = new MockParticipantManager(ZK_ADDR, clusterName, instanceName);
+ participants[i].syncStart();
+ }
+
+ ZKHelixDataAccessor accessor =
+ new ZKHelixDataAccessor(clusterName, new ZkBaseDataAccessor<ZNRecord>(_gZkClient));
+ PropertyKey.Builder keyBuilder = accessor.keyBuilder();
+
+ CustomizedStateConfig config = new CustomizedStateConfig();
+ List<String> aggregationEnabledTypes = new ArrayList<>();
+ aggregationEnabledTypes.add(CUSTOMIZED_STATE_NAME);
+ config.setAggregationEnabledTypes(aggregationEnabledTypes);
+
+ accessor.setProperty(keyBuilder.customizedStateConfig(), config);
+
+ CustomizedState customizedState = new CustomizedState(RESOURCE_NAME);
+ customizedState.setState(PARTITION_NAME, "STARTED");
+ accessor.setProperty(
+ keyBuilder.customizedState(INSTANCE_NAME1, CUSTOMIZED_STATE_NAME, RESOURCE_NAME),
+ customizedState);
+
+ CustomizedView customizedView = null;
+ int i = 0;
+ while (true) {
+ sleep(1000);
+ try {
+ customizedView = accessor.getProperty(keyBuilder.customizedView(CUSTOMIZED_STATE_NAME, RESOURCE_NAME));
+ Map<String, String> stateMap = customizedView.getRecord().getMapField(PARTITION_NAME);
+ if (stateMap.get(INSTANCE_NAME1).equals("STARTED")) {
+ System.out.println("succeed");
+ break;
+ }
+ } catch (Exception e) {
+ i ++;
+ if (i >= 10) {
+ Assert.fail("The customized view is not correct");
+ }
+ }
+ }
+
+ // disable controller
+ ZKHelixAdmin admin = new ZKHelixAdmin(_gZkClient);
+ admin.enableCluster(clusterName, false);
+ ZkTestHelper.tryWaitZkEventsCleaned(controller.getZkClient());
+
+ // drop resource
+ admin.dropResource(clusterName, RESOURCE_NAME);
+
+ // delete customized state manually, controller shall remove customized view when cluster is enabled again
+
+ accessor.removeProperty(
+ keyBuilder.customizedState(INSTANCE_NAME1, CUSTOMIZED_STATE_NAME, RESOURCE_NAME));
+ accessor.removeProperty(
+ keyBuilder.currentState(INSTANCE_NAME2, CUSTOMIZED_STATE_NAME, RESOURCE_NAME));
+
+ // re-enable controller shall remove orphan external view
+ // System.out.println("re-enabling controller");
+ admin.enableCluster(clusterName, true);
+
+ customizedView = null;
+ for (i = 0; i < 10; i++) {
+ sleep(100);
+ customizedView = accessor.getProperty(keyBuilder.customizedView(CUSTOMIZED_STATE_NAME));
+ if (customizedView == null) {
+ break;
+ }
+ }
+
+ Assert.assertNull(customizedView,
+ "customized view for TestDB0 should be removed, but was: " + customizedView);
+
+ // clean up
+ controller.syncStop();
+ for (i = 0; i < n; i++) {
+ participants[i].syncStop();
+ }
+ TestHelper.dropCluster(clusterName, _gZkClient);
+
+ System.out.println("END " + clusterName + " at " + new Date(System.currentTimeMillis()));
+ }
+
Review comment:
Remove empty line?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r394043083
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/ResourceControllerDataProvider.java
##########
@@ -118,7 +118,12 @@ public synchronized void refresh(HelixDataAccessor accessor) {
if (propertyRefreshed.contains(HelixConstants.ChangeType.IDEAL_STATE)
|| propertyRefreshed.contains(HelixConstants.ChangeType.LIVE_INSTANCE)
|| propertyRefreshed.contains(HelixConstants.ChangeType.INSTANCE_CONFIG)
+<<<<<<< HEAD
Review comment:
Fix conflicts :)
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851:
Add customized view computation and modify Helix generic controller to
include new stage
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r389067026
##########
File path: helix-core/src/main/java/org/apache/helix/common/caches/CustomizedStateCache.java
##########
@@ -49,11 +49,15 @@ public CustomizedStateCache(ControlContextProvider contextProvider) {
Map<String, LiveInstance> liveInstanceMap) {
Set<PropertyKey> participantStateKeys = new HashSet<>();
PropertyKey.Builder keyBuilder = accessor.keyBuilder();
- Set<String> restrictedKeys = new HashSet<>(
- accessor.getProperty(accessor.keyBuilder().customizedStateAggregationConfig()).getRecord()
- .getListFields().get(
- CustomizedStateAggregationConfig.CustomizedStateAggregationProperty.AGGREGATION_ENABLED_TYPES
- .name()));
+
+ Set<String> restrictedKeys = new HashSet<>();
+ if (accessor.getProperty(accessor.keyBuilder().customizedStateAggregationConfig()) != null) {
+ restrictedKeys = new HashSet<>(
+ accessor.getProperty(accessor.keyBuilder().customizedStateAggregationConfig()).getRecord()
+ .getListFields().get(
+ CustomizedStateAggregationConfig.CustomizedStateAggregationProperty.AGGREGATION_ENABLED_TYPES
Review comment:
yes.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r396797274
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1233,4 +1334,18 @@ private void initPipeline(Thread eventThread, BaseControllerDataProvider cache)
eventThread.setDaemon(true);
eventThread.start();
}
-}
\ No newline at end of file
+
+ private List<String> getEnabledCustomizedStates(HelixManager manager) {
+ CustomizedStateConfig customizedStateConfig =
+ manager.getConfigAccessor().getCustomizedStateConfig(_clusterName);
+ if (customizedStateConfig != null) {
+ return customizedStateConfig.getAggregationEnabledTypes();
+ }
+ return Collections.emptyList();
+ }
+
+ private void addCustomizedStateListeners(HelixManager manager, String customizedState,
Review comment:
What is this?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r388615034
##########
File path: helix-core/src/main/java/org/apache/helix/common/caches/CustomizedViewCache.java
##########
@@ -139,6 +139,17 @@ private PropertyKey customizedViewKey(PropertyKey.Builder keyBuilder, String res
return Collections.unmodifiableMap(_customizedViewMap);
}
+ /**
+ * Remove dead customized views from map
+ * @param resourceNames
+ */
+
+ public void removeCustomizedView(List<String> resourceNames) {
Review comment:
Do you think we need sync control on all the map operations here?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r388620151
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -899,6 +937,48 @@ public void onResourceConfigChange(
.info("END: GenericClusterController.onResourceConfigChange() for cluster " + _clusterName);
}
+ @Override
+ @PreFetch(enabled = false)
+ public void onCustomizedStateAggregationConfigChange(
+ CustomizedStateAggregationConfig customizedStateAggregationConfig,
+ NotificationContext context) {
+ HelixManager helixManager = context.getManager();
+ // add customized state listeners for existing instances
+ List<String> customizedStates = new ArrayList<>();
+
+ if (helixManager.getConfigAccessor()
+ .getCustomizedStateAggregationConfig(_clusterName) != null) {
+ customizedStates = helixManager.getConfigAccessor()
+ .getCustomizedStateAggregationConfig(_clusterName).getAggregationEnabledTypes();
+ }
+
+ Map<String, LiveInstance> liveInstanceMap = helixManager.getHelixDataAccessor()
+ .getChildValuesMap(helixManager.getHelixDataAccessor().keyBuilder().liveInstances());
+ List<String> liveInstances = liveInstanceMap.values().stream()
+ .map(liveInstance -> liveInstance.getInstanceName()).collect(Collectors.toList());
+
+ for (String customizedState : customizedStates) {
+ for (String instance : liveInstances) {
+ try {
+ helixManager.addCustomizedStateChangeListener(this, instance, customizedState);
Review comment:
Can these logic be pushed to the event handling section?
1. Any error here will break the ZK thread. There will be a very ugly exception output.
2. The extended latency will impact the whole controller performance.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r388613154
##########
File path: helix-core/src/main/java/org/apache/helix/PropertyPathBuilder.java
##########
@@ -268,6 +271,18 @@ public static String externalView(String clusterName, String resourceName) {
return String.format("/%s/EXTERNALVIEW/%s", clusterName, resourceName);
}
+ public static String customizedView(String clusterName) {
+ return String.format("/%s/CUSTOMIZEDVIEW", clusterName);
+ }
+
+ public static String customizedView(String clusterName, String customizedStateName) {
+ return String.format("/%s/EXTERNALVIEW/%s", clusterName, customizedStateName);
Review comment:
Why it's "EXTERNALVIEW"?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r394043654
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1233,4 +1316,24 @@ private void initPipeline(Thread eventThread, BaseControllerDataProvider cache)
eventThread.setDaemon(true);
eventThread.start();
}
+
+ private List<String> getEnabledCustomizedStates(HelixManager manager) {
+ CustomizedStateConfig customizedStateConfig =
+ manager.getConfigAccessor().getCustomizedStateConfig(_clusterName);
+ if (customizedStateConfig != null) {
+ return customizedStateConfig.getAggregationEnabledTypes();
+ }
+ return new ArrayList<>();
Review comment:
`Collections.emptyList()` may be more lightweight as a returning empty list?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r388618049
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -431,16 +443,31 @@ private static PipelineRegistry createDefaultRegistry(String pipelineName) {
Pipeline autoExitMaintenancePipeline = new Pipeline(pipelineName);
autoExitMaintenancePipeline.addStage(new MaintenanceRecoveryStage());
- registry.register(ClusterEventType.IdealStateChange, dataRefresh, dataPreprocess, rebalancePipeline);
- registry.register(ClusterEventType.CurrentStateChange, dataRefresh, dataPreprocess, externalViewPipeline, rebalancePipeline);
- registry.register(ClusterEventType.InstanceConfigChange, dataRefresh, dataPreprocess, rebalancePipeline);
- registry.register(ClusterEventType.ResourceConfigChange, dataRefresh, dataPreprocess, rebalancePipeline);
- registry.register(ClusterEventType.ClusterConfigChange, dataRefresh, autoExitMaintenancePipeline, dataPreprocess, rebalancePipeline);
- registry.register(ClusterEventType.LiveInstanceChange, dataRefresh, autoExitMaintenancePipeline, liveInstancePipeline, dataPreprocess, externalViewPipeline, rebalancePipeline);
- registry.register(ClusterEventType.MessageChange, dataRefresh, dataPreprocess, rebalancePipeline);
- registry.register(ClusterEventType.Resume, dataRefresh, dataPreprocess, externalViewPipeline, rebalancePipeline);
- registry.register(ClusterEventType.PeriodicalRebalance, dataRefresh, autoExitMaintenancePipeline, dataPreprocess, externalViewPipeline, rebalancePipeline);
- registry.register(ClusterEventType.OnDemandRebalance, dataRefresh, autoExitMaintenancePipeline, dataPreprocess, externalViewPipeline, rebalancePipeline);
+ registry.register(ClusterEventType.IdealStateChange, dataRefresh, dataPreprocess,
+ rebalancePipeline);
+ registry.register(ClusterEventType.CurrentStateChange, dataRefresh, dataPreprocess,
+ externalViewPipeline, rebalancePipeline);
+ registry.register(ClusterEventType.InstanceConfigChange, dataRefresh, dataPreprocess,
+ rebalancePipeline);
+ registry.register(ClusterEventType.ResourceConfigChange, dataRefresh, dataPreprocess,
+ rebalancePipeline);
+ registry.register(ClusterEventType.ClusterConfigChange, dataRefresh,
+ autoExitMaintenancePipeline, dataPreprocess, rebalancePipeline);
+ registry.register(ClusterEventType.LiveInstanceChange, dataRefresh,
+ autoExitMaintenancePipeline, liveInstancePipeline, dataPreprocess, externalViewPipeline, customizedViewPipeline,
+ rebalancePipeline);
+ registry.register(ClusterEventType.MessageChange, dataRefresh, dataPreprocess,
+ rebalancePipeline);
+ registry.register(ClusterEventType.Resume, dataRefresh, dataPreprocess, externalViewPipeline,
+ rebalancePipeline);
+ registry.register(ClusterEventType.PeriodicalRebalance, dataRefresh,
+ autoExitMaintenancePipeline, dataPreprocess, externalViewPipeline, rebalancePipeline);
+ registry.register(ClusterEventType.OnDemandRebalance, dataRefresh,
+ autoExitMaintenancePipeline, dataPreprocess, externalViewPipeline, rebalancePipeline);
+ registry.register(ClusterEventType.CustomizedStateChange, dataRefresh, dataPreprocess,
Review comment:
This is problematic. If you refresh all the data, including current state, idealstate etc., then you don't do rebalancePipeline, the controller might fail to react on some critical event.
I think we need to separate the custiomizedState read stage from the dataPreprocess (maybe also data refresh).
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r394689942
##########
File path: helix-core/src/test/java/org/apache/helix/integration/TestZkCallbackHandlerLeak.java
##########
@@ -247,7 +247,7 @@ public boolean verify() throws Exception {
int controllerHandlerNb = controller.getHandlers().size();
int particHandlerNb = participantManager.getHandlers().size();
- Assert.assertEquals(controllerHandlerNb, 7 + 2 * n,
+ Assert.assertEquals(controllerHandlerNb, 8 + 2 * n,
"HelixController should have 10 (6+2n) callback handlers for 2 participant, but was "
Review comment:
Same here.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r396799467
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -775,6 +811,39 @@ public void onStateChange(String instanceName, List<CurrentState> statesInfo,
logger.info("END: GenericClusterController.onStateChange()");
}
+ @Override
+ @PreFetch(enabled = false)
+ public void onCustomizedStateRootChange(String instanceName, NotificationContext changeContext) {
+ logger.info("START: GenericClusterController.onCustomizedStateRootChange()");
+ notifyCaches(changeContext, ChangeType.CUSTOMIZED_STATE_ROOT);
Review comment:
We don't need to notify the cache, right?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on issue #851: Modify Helix generic
controller to include new stage for customized view aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on issue #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#issuecomment-601939430
> One more thing, the added test cases are not enough. Please add tests for the following features:
>
> 1. We need to add a case in the test listener leakage for the new listener introduced.
This one is tested in TestZkCallbackHandlerLeak.
> 2. Admin change to create the new paths.
This one is tested in TestZkHelixAdmin.
> 3. In the integration test, please test adjusting the view config and see if the view changed accordingly. Moreover, we should use the customized view provider to test.
Tested in TestComputeAndCleanupCustomizedView.
The provider has unit test checked in, and the end to end integration test will cover from provider to router, which will be checked in by @mgao0.
> 4. Try to cover some error case, for example, instances are not reporting the configured type. Or 2 instances are reporting different sets of types.
Added test cases in TestComputeAndCleanupCustomizedView
> Please try to cover all the use cases that we mentioned in the design doc.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r394036264
##########
File path: helix-core/src/test/java/org/apache/helix/integration/TestComputeAndCleanupCustomizedView.java
##########
@@ -0,0 +1,166 @@
+package org.apache.helix.integration;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.ArrayList;
+import java.util.Date;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.TestHelper;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.ZkTestHelper;
+import org.apache.helix.ZkUnitTestBase;
+import org.apache.helix.integration.manager.ClusterControllerManager;
+import org.apache.helix.integration.manager.MockParticipantManager;
+import org.apache.helix.manager.zk.ZKHelixAdmin;
+import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.apache.helix.model.CustomizedState;
+import org.apache.helix.model.CustomizedStateConfig;
+import org.apache.helix.model.CustomizedView;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import static java.lang.Thread.*;
Review comment:
No need to import `*`?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r394036869
##########
File path: helix-core/src/test/java/org/apache/helix/controller/stages/TestCustomizedViewStage.java
##########
@@ -0,0 +1,105 @@
+package org.apache.helix.controller.stages;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixManager;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.TestHelper;
+import org.apache.helix.ZkUnitTestBase;
+import org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.controller.pipeline.Pipeline;
+import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.apache.helix.model.CustomizedState;
+import org.apache.helix.model.CustomizedStateConfig;
+import org.apache.helix.model.CustomizedView;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+public class TestCustomizedViewStage extends ZkUnitTestBase {
+ private final String RESOURCE_NAME = "testResourceName";
+ private final String PARTITION_NAME = "testResourceName_0";
+ private final String CUSTOMIZED_STATE_NAME = "customizedState1";
+ private final String INSTANCE_NAME = "localhost_1";
+
+ @Test
+ public void testCachedCustomizedViews() throws Exception {
+ String clusterName = "CLUSTER_" + TestHelper.getTestMethodName();
+
+ HelixDataAccessor accessor =
+ new ZKHelixDataAccessor(clusterName, new ZkBaseDataAccessor<>(_gZkClient));
+ HelixManager manager = new DummyClusterManager(clusterName, accessor);
+
+ setupLiveInstances(clusterName, new int[]{0, 1});
+ setupStateModel(clusterName);
+
+ ClusterEvent event = new ClusterEvent(ClusterEventType.Unknown);
+ ResourceControllerDataProvider cache = new ResourceControllerDataProvider(clusterName);
+ event.addAttribute(AttributeName.helixmanager.name(), manager);
+ event.addAttribute(AttributeName.ControllerDataProvider.name(), cache);
+
+ CustomizedStateConfig config = new CustomizedStateConfig();
+ List<String> aggregationEnabledTypes = new ArrayList<>();
+ aggregationEnabledTypes.add(CUSTOMIZED_STATE_NAME);
+ config.setAggregationEnabledTypes(aggregationEnabledTypes);
+
+ PropertyKey.Builder keyBuilder = accessor.keyBuilder();
+ accessor.setProperty(keyBuilder.customizedStateConfig(), config);
+
+ CustomizedState customizedState = new CustomizedState(RESOURCE_NAME);
+ customizedState.setState(PARTITION_NAME, "STARTED");
+ accessor
+ .setProperty(keyBuilder.customizedState(INSTANCE_NAME, "customizedState1", RESOURCE_NAME),
+ customizedState);
+
+ CustomizedViewAggregateStage customizedViewComputeStage = new CustomizedViewAggregateStage();
+ Pipeline dataRefresh = new Pipeline();
+ dataRefresh.addStage(new ReadClusterDataStage());
+ runPipeline(event, dataRefresh);
+ runStage(event, new ResourceComputationStage());
+ runStage(event, new CustomizedStateComputationStage());
+ runStage(event, customizedViewComputeStage);
+ Assert.assertEquals(cache.getCustomizedViewCacheMap().values(),
+ accessor.getChildValues(accessor.keyBuilder().customizedViews()));
+
+ // Assure there is no customized view got updated
+ List<CustomizedView> oldCustomizedViews =
+ accessor.getChildValues(accessor.keyBuilder().customizedViews());
+ runStage(event, customizedViewComputeStage);
+ List<CustomizedView> newCustomizedViews =
+ accessor.getChildValues(accessor.keyBuilder().customizedViews());
+ Assert.assertEquals(oldCustomizedViews, newCustomizedViews);
Review comment:
Are you sure the ordering in both lists is the same? If the elements are the same but ordering is different, this test would fail. I suggest put them into Sets.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r391177042
##########
File path: helix-core/src/main/java/org/apache/helix/common/caches/CustomizedStateCache.java
##########
@@ -46,14 +46,9 @@ public CustomizedStateCache(ControlContextProvider contextProvider) {
@Override
protected Set<PropertyKey> PopulateParticipantKeys(HelixDataAccessor accessor,
- Map<String, LiveInstance> liveInstanceMap) {
+ Map<String, LiveInstance> liveInstanceMap, Set<String> restrictedKeys) {
Review comment:
I think we discussed to put the restrictedKeys to the CustomizedStateCache constructor as a final field. So you don't need to add this to these methods? The downside of the current design is that the current state cache will have to keep the unnecessary input.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r388614743
##########
File path: helix-core/src/main/java/org/apache/helix/common/caches/CustomizedStateCache.java
##########
@@ -49,11 +49,15 @@ public CustomizedStateCache(ControlContextProvider contextProvider) {
Map<String, LiveInstance> liveInstanceMap) {
Set<PropertyKey> participantStateKeys = new HashSet<>();
PropertyKey.Builder keyBuilder = accessor.keyBuilder();
- Set<String> restrictedKeys = new HashSet<>(
- accessor.getProperty(accessor.keyBuilder().customizedStateAggregationConfig()).getRecord()
- .getListFields().get(
- CustomizedStateAggregationConfig.CustomizedStateAggregationProperty.AGGREGATION_ENABLED_TYPES
- .name()));
+
+ Set<String> restrictedKeys = new HashSet<>();
+ if (accessor.getProperty(accessor.keyBuilder().customizedStateAggregationConfig()) != null) {
+ restrictedKeys = new HashSet<>(
+ accessor.getProperty(accessor.keyBuilder().customizedStateAggregationConfig()).getRecord()
+ .getListFields().get(
+ CustomizedStateAggregationConfig.CustomizedStateAggregationProperty.AGGREGATION_ENABLED_TYPES
Review comment:
So if the config is empty, the default behavior is aggregating nothing, right?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r394041792
##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/CustomizedViewAggregateStage.java
##########
@@ -0,0 +1,171 @@
+package org.apache.helix.controller.stages;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.HelixManager;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.common.caches.CustomizedViewCache;
+import org.apache.helix.controller.LogUtil;
+import org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.controller.pipeline.AbstractAsyncBaseStage;
+import org.apache.helix.controller.pipeline.AsyncWorkerType;
+import org.apache.helix.controller.pipeline.StageException;
+import org.apache.helix.model.CustomizedView;
+import org.apache.helix.model.Partition;
+import org.apache.helix.model.Resource;
+import org.apache.helix.monitoring.mbeans.ClusterStatusMonitor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class CustomizedViewAggregateStage extends AbstractAsyncBaseStage {
+ private static Logger LOG = LoggerFactory.getLogger(CustomizedViewAggregateStage.class);
+
+ @Override
+ public AsyncWorkerType getAsyncWorkerType() {
+ return AsyncWorkerType.CustomizedStateViewComputeWorker;
+ }
+
+ @Override
+ public void execute(final ClusterEvent event) throws Exception {
Review comment:
The implementation in this method is too long. Can we try to split it into several private methods like:
```
removeStaleTypes()
updateCustomizedView()
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r396797454
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1233,4 +1334,18 @@ private void initPipeline(Thread eventThread, BaseControllerDataProvider cache)
eventThread.setDaemon(true);
eventThread.start();
}
-}
\ No newline at end of file
+
+ private List<String> getEnabledCustomizedStates(HelixManager manager) {
Review comment:
It seems there is no usage of this private method.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r388620766
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1072,6 +1152,25 @@ protected void checkLiveInstancesObservation(List<LiveInstance> liveInstances,
}
}
+ for (String instance : curInstances.keySet()) {
Review comment:
Why we need another loop looks exactly the same as the addMessageListener logic? Can we just do the work in the same loop?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851:
Add customized view computation and modify Helix generic controller to
include new stage
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r389976424
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
##########
@@ -286,6 +292,27 @@ private void refreshResourceConfig(final HelixDataAccessor accessor,
}
}
+ private void refreshCustomizedStateAggregationConfig(final HelixDataAccessor accessor,
Review comment:
As the customized state config is a cluster level config, and following the current pattern, I feel it is more natural to put it together with cluster config, resource config, etc.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r388626886
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
##########
@@ -286,6 +292,27 @@ private void refreshResourceConfig(final HelixDataAccessor accessor,
}
}
+ private void refreshCustomizedStateAggregationConfig(final HelixDataAccessor accessor,
Review comment:
This logic should be resource controller data provider only, I think.
It is not required by TF.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r398157359
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -122,7 +124,7 @@
// TODO: make this be per _manager or per _listener instaed of per callbackHandler -- Lei
private CallbackProcessor _batchCallbackProcessor;
private boolean _watchChild = true; // Whether we should subscribe to the child znode's data
- // change.
+ // change.
Review comment:
This is because the previous code was not correctly formatted in the switch section.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851:
Add customized view computation and modify Helix generic controller to
include new stage
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r389473129
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
##########
@@ -96,13 +99,15 @@
// Special caches
private CurrentStateCache _currentStateCache;
+ private CustomizedStateCache _customizedStateCache;
private InstanceMessagesCache _instanceMessagesCache;
// Other miscellaneous caches
private Map<String, Long> _instanceOfflineTimeMap;
private Map<String, Map<String, String>> _idealStateRuleMap;
private Map<String, Map<String, Set<String>>> _disabledInstanceForPartitionMap = new HashMap<>();
private Set<String> _disabledInstanceSet = new HashSet<>();
+ private Set<String> _aggregationEnabledTypes = new HashSet<>();
Review comment:
I added the aggregationEnableType back, as it will break some test. The main reason is that if we put the accessing to ZK inside customized state cache, it will break some assumption in the mock zk Helix accessor about refresh logic, and the calculation for "config" type would be wrong.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r398144033
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -813,23 +817,36 @@ public void onStateChange(String instanceName, List<CurrentState> statesInfo,
@Override
@PreFetch(enabled = false)
- public void onCustomizedStateRootChange(String instanceName, NotificationContext changeContext) {
+ public void onCustomizedStateRootChange(String instanceName, List<String> customizedStateTypes,
+ NotificationContext changeContext) {
logger.info("START: GenericClusterController.onCustomizedStateRootChange()");
- notifyCaches(changeContext, ChangeType.CUSTOMIZED_STATE_ROOT);
HelixManager manager = changeContext.getManager();
- List<String> customizedStateTypes =
- manager.getHelixDataAccessor().getChildNames(
- manager.getHelixDataAccessor().keyBuilder().customizedStatesRoot(instanceName));
+ Builder keyBuilder = new Builder(manager.getClusterName());
- for (String customizedState : customizedStateTypes) {
- try {
- manager.addCustomizedStateChangeListener(this, instanceName, customizedState);
- logger.info(
- manager.getInstanceName() + " added customized state listener for " + instanceName
- + ", listener: " + this);
- } catch (Exception e) {
- logger.error("Fail to add customized state listener for instance: " + instanceName, e);
+ synchronized (_lastSeenCustomizedStateTypes) {
+ Set<String> lastSeenCustomizedStateTypes = _lastSeenCustomizedStateTypes.get();
Review comment:
Then please also carry over the TODO part.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r394690807
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1072,6 +1146,15 @@ protected void checkLiveInstancesObservation(List<LiveInstance> liveInstances,
}
}
+ List<String> customizedStates = getEnabledCustomizedStates(manager);
+ for (String customizedState: customizedStates) {
+ for (String instance : curInstances.keySet()) {
+ if (lastInstances == null || !lastInstances.containsKey(instance)) {
+ addCustomizedStateListeners(manager, customizedState, instance);
Review comment:
When you have add, we should have a corresponding remove to prevent listener leakage.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r396866721
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -569,12 +570,19 @@ public void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
});
}
+ @Override
+ public void addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
+ String instanceName) throws Exception {
+ addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
+ ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ }
+
@Override
public void addCustomizedStateChangeListener(CustomizedStateChangeListener listener,
- String instanceName, String customizedStateName) throws Exception {
- addListener(listener, new Builder(_clusterName).customizedStates(instanceName, customizedStateName),
- ChangeType.CUSTOMIZED_STATE, new EventType[] { EventType.NodeChildrenChanged
- });
+ String instanceName, String customizedStateType) throws Exception {
Review comment:
Yeah, agree. @jiajunwang and I will look at how to merge the two layers later.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851:
Add customized view computation and modify Helix generic controller to
include new stage
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r389074906
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
##########
@@ -96,13 +99,15 @@
// Special caches
private CurrentStateCache _currentStateCache;
+ private CustomizedStateCache _customizedStateCache;
private InstanceMessagesCache _instanceMessagesCache;
// Other miscellaneous caches
private Map<String, Long> _instanceOfflineTimeMap;
private Map<String, Map<String, String>> _idealStateRuleMap;
private Map<String, Map<String, Set<String>>> _disabledInstanceForPartitionMap = new HashMap<>();
private Set<String> _disabledInstanceSet = new HashSet<>();
+ private Set<String> _aggregationEnabledTypes = new HashSet<>();
Review comment:
I've removed it, it used to be used by customized state cache, but now we've changed that cache and access the customized state config directly in the cache. So we do not need this variable.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r397407664
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -259,45 +266,52 @@ private void parseListenerProperties() {
Class listenerClass = null;
switch (_changeType) {
- case IDEAL_STATE:
- listenerClass = IdealStateChangeListener.class;
- break;
- case INSTANCE_CONFIG:
- if (_listener instanceof ConfigChangeListener) {
+ case IDEAL_STATE:
+ listenerClass = IdealStateChangeListener.class;
+ break;
+ case INSTANCE_CONFIG:
+ if (_listener instanceof ConfigChangeListener) {
+ listenerClass = ConfigChangeListener.class;
+ } else if (_listener instanceof InstanceConfigChangeListener) {
+ listenerClass = InstanceConfigChangeListener.class;
+ }
+ break;
+ case CLUSTER_CONFIG:
+ listenerClass = ClusterConfigChangeListener.class;
+ break;
+ case RESOURCE_CONFIG:
+ listenerClass = ResourceConfigChangeListener.class;
+ break;
+ case CUSTOMIZED_STATE_CONFIG:
+ listenerClass = CustomizedStateConfigChangeListener.class;
+ break;
+ case CONFIG:
listenerClass = ConfigChangeListener.class;
- } else if (_listener instanceof InstanceConfigChangeListener) {
- listenerClass = InstanceConfigChangeListener.class;
- }
- break;
- case CLUSTER_CONFIG:
- listenerClass = ClusterConfigChangeListener.class;
- break;
- case RESOURCE_CONFIG:
- listenerClass = ResourceConfigChangeListener.class;
- break;
- case CONFIG:
- listenerClass = ConfigChangeListener.class;
- break;
- case LIVE_INSTANCE:
- listenerClass = LiveInstanceChangeListener.class;
- break;
- case CURRENT_STATE:
- listenerClass = CurrentStateChangeListener.class;
- ;
- break;
- case MESSAGE:
- case MESSAGES_CONTROLLER:
- listenerClass = MessageListener.class;
- break;
- case EXTERNAL_VIEW:
- case TARGET_EXTERNAL_VIEW:
- listenerClass = ExternalViewChangeListener.class;
- break;
- case CUSTOMIZED_VIEW:
- listenerClass = CustomizedViewChangeListener.class;
- break;
- case CONTROLLER:
- listenerClass = ControllerChangeListener.class;
+ break;
+ case LIVE_INSTANCE:
+ listenerClass = LiveInstanceChangeListener.class;
+ break;
+ case CURRENT_STATE:
+ listenerClass = CurrentStateChangeListener.class;
+ break;
+ case CUSTOMIZED_STATE_ROOT:
+ listenerClass = CustomizedStateRootChangeListener.class;
Review comment:
Yeah, I double checked later. It is not doable given the current infrastructure only take the first method it found.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r396824926
##########
File path: helix-core/src/main/java/org/apache/helix/api/listeners/CustomizedStateRootChangeListener.java
##########
@@ -0,0 +1,36 @@
+package org.apache.helix.api.listeners;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.helix.NotificationContext;
+
+
+/**
+ * Interface to implement to respond to changes in the root path of customized state
+ */
+public interface CustomizedStateRootChangeListener {
+
+ /**
+ * Invoked when root path customized state changes
+ * @param instanceName name of the instance whose state changed
+ * @param changeContext the change event and state
+ */
+ void onCustomizedStateRootChange(String instanceName, NotificationContext changeContext);
Review comment:
Shall this method be passed a list of types in case anyone defined it with PreFetch == true?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r394680179
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -431,16 +443,31 @@ private static PipelineRegistry createDefaultRegistry(String pipelineName) {
Pipeline autoExitMaintenancePipeline = new Pipeline(pipelineName);
autoExitMaintenancePipeline.addStage(new MaintenanceRecoveryStage());
- registry.register(ClusterEventType.IdealStateChange, dataRefresh, dataPreprocess, rebalancePipeline);
- registry.register(ClusterEventType.CurrentStateChange, dataRefresh, dataPreprocess, externalViewPipeline, rebalancePipeline);
- registry.register(ClusterEventType.InstanceConfigChange, dataRefresh, dataPreprocess, rebalancePipeline);
- registry.register(ClusterEventType.ResourceConfigChange, dataRefresh, dataPreprocess, rebalancePipeline);
- registry.register(ClusterEventType.ClusterConfigChange, dataRefresh, autoExitMaintenancePipeline, dataPreprocess, rebalancePipeline);
- registry.register(ClusterEventType.LiveInstanceChange, dataRefresh, autoExitMaintenancePipeline, liveInstancePipeline, dataPreprocess, externalViewPipeline, rebalancePipeline);
- registry.register(ClusterEventType.MessageChange, dataRefresh, dataPreprocess, rebalancePipeline);
- registry.register(ClusterEventType.Resume, dataRefresh, dataPreprocess, externalViewPipeline, rebalancePipeline);
- registry.register(ClusterEventType.PeriodicalRebalance, dataRefresh, autoExitMaintenancePipeline, dataPreprocess, externalViewPipeline, rebalancePipeline);
- registry.register(ClusterEventType.OnDemandRebalance, dataRefresh, autoExitMaintenancePipeline, dataPreprocess, externalViewPipeline, rebalancePipeline);
+ registry.register(ClusterEventType.IdealStateChange, dataRefresh, dataPreprocess,
+ rebalancePipeline);
+ registry.register(ClusterEventType.CurrentStateChange, dataRefresh, dataPreprocess,
+ externalViewPipeline, rebalancePipeline);
+ registry.register(ClusterEventType.InstanceConfigChange, dataRefresh, dataPreprocess,
+ rebalancePipeline);
+ registry.register(ClusterEventType.ResourceConfigChange, dataRefresh, dataPreprocess,
+ rebalancePipeline);
+ registry.register(ClusterEventType.ClusterConfigChange, dataRefresh,
+ autoExitMaintenancePipeline, dataPreprocess, rebalancePipeline);
+ registry.register(ClusterEventType.LiveInstanceChange, dataRefresh,
+ autoExitMaintenancePipeline, liveInstancePipeline, dataPreprocess, externalViewPipeline, customizedViewPipeline,
+ rebalancePipeline);
+ registry.register(ClusterEventType.MessageChange, dataRefresh, dataPreprocess,
+ rebalancePipeline);
+ registry.register(ClusterEventType.Resume, dataRefresh, dataPreprocess, externalViewPipeline,
+ rebalancePipeline);
+ registry.register(ClusterEventType.PeriodicalRebalance, dataRefresh,
+ autoExitMaintenancePipeline, dataPreprocess, externalViewPipeline, rebalancePipeline);
+ registry.register(ClusterEventType.OnDemandRebalance, dataRefresh,
+ autoExitMaintenancePipeline, dataPreprocess, externalViewPipeline, rebalancePipeline);
+ registry.register(ClusterEventType.CustomizedStateChange, dataRefresh, dataPreprocess,
Review comment:
The issue has been resolved with rebalancePipeline at the end. Please add a TODO here to note it is not efficient, and we should improve this by splitting the pipeline or controller roles to multiple hosts.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r394043654
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1233,4 +1316,24 @@ private void initPipeline(Thread eventThread, BaseControllerDataProvider cache)
eventThread.setDaemon(true);
eventThread.start();
}
+
+ private List<String> getEnabledCustomizedStates(HelixManager manager) {
+ CustomizedStateConfig customizedStateConfig =
+ manager.getConfigAccessor().getCustomizedStateConfig(_clusterName);
+ if (customizedStateConfig != null) {
+ return customizedStateConfig.getAggregationEnabledTypes();
+ }
+ return new ArrayList<>();
Review comment:
`Collections.singletonList()` may be more lightweight as a returning empty list?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r398114728
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -813,23 +817,36 @@ public void onStateChange(String instanceName, List<CurrentState> statesInfo,
@Override
@PreFetch(enabled = false)
- public void onCustomizedStateRootChange(String instanceName, NotificationContext changeContext) {
+ public void onCustomizedStateRootChange(String instanceName, List<String> customizedStateTypes,
+ NotificationContext changeContext) {
logger.info("START: GenericClusterController.onCustomizedStateRootChange()");
- notifyCaches(changeContext, ChangeType.CUSTOMIZED_STATE_ROOT);
HelixManager manager = changeContext.getManager();
- List<String> customizedStateTypes =
- manager.getHelixDataAccessor().getChildNames(
- manager.getHelixDataAccessor().keyBuilder().customizedStatesRoot(instanceName));
+ Builder keyBuilder = new Builder(manager.getClusterName());
- for (String customizedState : customizedStateTypes) {
- try {
- manager.addCustomizedStateChangeListener(this, instanceName, customizedState);
- logger.info(
- manager.getInstanceName() + " added customized state listener for " + instanceName
- + ", listener: " + this);
- } catch (Exception e) {
- logger.error("Fail to add customized state listener for instance: " + instanceName, e);
+ synchronized (_lastSeenCustomizedStateTypes) {
+ Set<String> lastSeenCustomizedStateTypes = _lastSeenCustomizedStateTypes.get();
+ for (String customizedState : customizedStateTypes) {
Review comment:
I made the change. However, I feel we actually do not need the prefetch logic for this event. It's redundant now. Please let me know.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r394681759
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -431,16 +443,35 @@ private static PipelineRegistry createDefaultRegistry(String pipelineName) {
Pipeline autoExitMaintenancePipeline = new Pipeline(pipelineName);
autoExitMaintenancePipeline.addStage(new MaintenanceRecoveryStage());
- registry.register(ClusterEventType.IdealStateChange, dataRefresh, dataPreprocess, rebalancePipeline);
- registry.register(ClusterEventType.CurrentStateChange, dataRefresh, dataPreprocess, externalViewPipeline, rebalancePipeline);
- registry.register(ClusterEventType.InstanceConfigChange, dataRefresh, dataPreprocess, rebalancePipeline);
- registry.register(ClusterEventType.ResourceConfigChange, dataRefresh, dataPreprocess, rebalancePipeline);
- registry.register(ClusterEventType.ClusterConfigChange, dataRefresh, autoExitMaintenancePipeline, dataPreprocess, rebalancePipeline);
- registry.register(ClusterEventType.LiveInstanceChange, dataRefresh, autoExitMaintenancePipeline, liveInstancePipeline, dataPreprocess, externalViewPipeline, rebalancePipeline);
- registry.register(ClusterEventType.MessageChange, dataRefresh, dataPreprocess, rebalancePipeline);
- registry.register(ClusterEventType.Resume, dataRefresh, dataPreprocess, externalViewPipeline, rebalancePipeline);
- registry.register(ClusterEventType.PeriodicalRebalance, dataRefresh, autoExitMaintenancePipeline, dataPreprocess, externalViewPipeline, rebalancePipeline);
- registry.register(ClusterEventType.OnDemandRebalance, dataRefresh, autoExitMaintenancePipeline, dataPreprocess, externalViewPipeline, rebalancePipeline);
+ registry.register(ClusterEventType.IdealStateChange, dataRefresh, dataPreprocess,
+ rebalancePipeline);
+ registry.register(ClusterEventType.CurrentStateChange, dataRefresh, dataPreprocess,
+ externalViewPipeline, rebalancePipeline);
+ registry.register(ClusterEventType.InstanceConfigChange, dataRefresh, dataPreprocess,
+ rebalancePipeline);
+ registry.register(ClusterEventType.ResourceConfigChange, dataRefresh, dataPreprocess,
+ rebalancePipeline);
+ registry
+ .register(ClusterEventType.ClusterConfigChange, dataRefresh, autoExitMaintenancePipeline,
+ dataPreprocess, rebalancePipeline);
+ registry
+ .register(ClusterEventType.LiveInstanceChange, dataRefresh, autoExitMaintenancePipeline,
+ liveInstancePipeline, dataPreprocess, externalViewPipeline, customizedViewPipeline,
+ rebalancePipeline);
+ registry
+ .register(ClusterEventType.MessageChange, dataRefresh, dataPreprocess, rebalancePipeline);
+ registry.register(ClusterEventType.Resume, dataRefresh, dataPreprocess, externalViewPipeline,
+ rebalancePipeline);
+ registry
+ .register(ClusterEventType.PeriodicalRebalance, dataRefresh, autoExitMaintenancePipeline,
+ dataPreprocess, externalViewPipeline, rebalancePipeline);
+ registry
+ .register(ClusterEventType.OnDemandRebalance, dataRefresh, autoExitMaintenancePipeline,
+ dataPreprocess, externalViewPipeline, rebalancePipeline);
+ registry.register(ClusterEventType.CustomizedStateChange, dataRefresh, dataPreprocess,
+ externalViewPipeline, customizedViewPipeline, rebalancePipeline);
+ registry.register(ClusterEventType.CustomizeStateAggregationConfigChange, dataRefresh,
Review comment:
I think externalViewPipeline is not necessary here. EV change will be eventually trigged by CS change.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851:
Add customized view computation and modify Helix generic controller to
include new stage
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r390011688
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1072,6 +1152,25 @@ protected void checkLiveInstancesObservation(List<LiveInstance> liveInstances,
}
}
+ for (String instance : curInstances.keySet()) {
Review comment:
The reason we need to add listeners in both functions is that only after customized state config is updated, the paths of individual states can be decided and listeners can be added to them. For example, after we add listener in live instance change, if we do not add listener in customized state config, we will miss the paths of the new customized state, and all the changes on this path will be missed until next live instance change. This is not preferred.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851: Add
customized view computation and modify Helix generic controller to include
new stage
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r391178860
##########
File path: helix-core/src/main/java/org/apache/helix/common/caches/ParticipantStateCache.java
##########
@@ -57,10 +57,11 @@ public ParticipantStateCache(ControlContextProvider controlContextProvider) {
* @param liveInstanceMap map of all liveInstances in cluster
* @return
*/
- public boolean refresh(HelixDataAccessor accessor, Map<String, LiveInstance> liveInstanceMap) {
+ public boolean refresh(HelixDataAccessor accessor, Map<String, LiveInstance> liveInstanceMap,
+ Set<String> restrictedKeys) {
Review comment:
Following the previous comment, I think this restrictedKeys can be a field only for the customizedStateCache class. As long as we input it in the constructor, we will be able to read it in the customizedStateCache class method. For this parent class, it does no need to know it.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #851: Modify
Helix generic controller to include new stage for customized view aggregation
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r395371018
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -775,6 +811,38 @@ public void onStateChange(String instanceName, List<CurrentState> statesInfo,
logger.info("END: GenericClusterController.onStateChange()");
}
+ @Override
Review comment:
Prefetch ?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r394688404
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ControllerManagerHelper.java
##########
@@ -102,6 +104,7 @@ public void removeListenersFromController(GenericHelixController controller) {
_manager.removeListener(keyBuilder.clusterConfig(), controller);
_manager.removeListener(keyBuilder.resourceConfigs(), controller);
_manager.removeListener(keyBuilder.instanceConfigs(), controller);
+ _manager.removeListener(keyBuilder.customizedStateConfig(), controller);
Review comment:
Please follow the reverse order in which the listeners are added.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r398115507
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -813,23 +817,36 @@ public void onStateChange(String instanceName, List<CurrentState> statesInfo,
@Override
@PreFetch(enabled = false)
- public void onCustomizedStateRootChange(String instanceName, NotificationContext changeContext) {
+ public void onCustomizedStateRootChange(String instanceName, List<String> customizedStateTypes,
+ NotificationContext changeContext) {
logger.info("START: GenericClusterController.onCustomizedStateRootChange()");
- notifyCaches(changeContext, ChangeType.CUSTOMIZED_STATE_ROOT);
HelixManager manager = changeContext.getManager();
- List<String> customizedStateTypes =
- manager.getHelixDataAccessor().getChildNames(
- manager.getHelixDataAccessor().keyBuilder().customizedStatesRoot(instanceName));
+ Builder keyBuilder = new Builder(manager.getClusterName());
- for (String customizedState : customizedStateTypes) {
- try {
- manager.addCustomizedStateChangeListener(this, instanceName, customizedState);
- logger.info(
- manager.getInstanceName() + " added customized state listener for " + instanceName
- + ", listener: " + this);
- } catch (Exception e) {
- logger.error("Fail to add customized state listener for instance: " + instanceName, e);
+ synchronized (_lastSeenCustomizedStateTypes) {
+ Set<String> lastSeenCustomizedStateTypes = _lastSeenCustomizedStateTypes.get();
Review comment:
I saw a todo here:
// TODO: remove the synchronization here once we move this update into dataCache.
synchronized (_lastSeenInstances) {
I'm not sure whether there's any pending task for this that could also apply here, so I kept it.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r396652209
##########
File path: helix-core/src/main/java/org/apache/helix/PropertyKey.java
##########
@@ -474,6 +474,15 @@ public PropertyKey currentState(String instanceName, String sessionId, String re
}
}
+ /**
+ * Get a property key associated with the root of {@link CustomizedState} of an instance
+ * @param instanceName
+ * @return {@link PropertyKey}
+ */
+ public PropertyKey customizedStatesRoot(String instanceName) {
Review comment:
I would prefer to call this method customizedState**s**, and the one for the specific state customizedState(without the s)
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851:
Modify Helix generic controller to include new stage for customized view
aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r396867483
##########
File path: helix-core/src/main/java/org/apache/helix/PropertyKey.java
##########
@@ -474,6 +474,15 @@ public PropertyKey currentState(String instanceName, String sessionId, String re
}
}
+ /**
+ * Get a property key associated with the root of {@link CustomizedState} of an instance
+ * @param instanceName
+ * @return {@link PropertyKey}
+ */
+ public PropertyKey customizedStatesRoot(String instanceName) {
Review comment:
This might cause more confusion as there're many places that have customizedStates as variable name. I fear it actually makes the later modification (combine two layers) more difficult. Have a "root" name here can help us know exactly where need to be changed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang merged pull request #851: Modify Helix generic
controller to include new stage for customized view aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851:
Add customized view computation and modify Helix generic controller to
include new stage
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #851: Add customized view computation and modify Helix generic controller to include new stage
URL: https://github.com/apache/helix/pull/851#discussion_r390022436
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1072,6 +1152,25 @@ protected void checkLiveInstancesObservation(List<LiveInstance> liveInstances,
}
}
+ for (String instance : curInstances.keySet()) {
+ if (lastInstances == null || !lastInstances.containsKey(instance)) {
+ try {
Review comment:
Please see the updated code.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org