[GitHub] [helix] jiajunwang commented on a change in pull request #510: Fixing rebalance cache issue and stablize the tests.
jiajunwang commented on a change in pull request #510: Fixing rebalance cache issue and stablize the tests. URL: https://github.com/apache/helix/pull/510#discussion_r335104513 ## File path: helix-core/src/test/java/org/apache/helix/integration/rebalancer/TestMixedModeAutoRebalance.java ## @@ -211,7 +211,6 @@ public void testUserDefinedPreferenceListsInFullAutoWithErrors() throws Exceptio } return false; }, 2000); -Assert.assertTrue(_clusterVerifier.verify(3000)); Review comment: Because we adjust the behavior of strict ev verifier again in this PR. Basically, change it back to verify based on the preference list. So it will never converge to mapping with ERROR state. Note that is changing the behavior back to fit the master branch 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #510: Fixing rebalance cache issue and stablize the tests.
jiajunwang commented on a change in pull request #510: Fixing rebalance cache
issue and stablize the tests.
URL: https://github.com/apache/helix/pull/510#discussion_r335103680
##
File path:
helix-core/src/main/java/org/apache/helix/controller/dataproviders/ResourceControllerDataProvider.java
##
@@ -124,7 +124,8 @@ public synchronized void refresh(HelixDataAccessor
accessor) {
if (changedTypes.contains(HelixConstants.ChangeType.IDEAL_STATE)
|| changedTypes.contains(HelixConstants.ChangeType.LIVE_INSTANCE)
|| changedTypes.contains(HelixConstants.ChangeType.INSTANCE_CONFIG)
-|| changedTypes.contains(HelixConstants.ChangeType.RESOURCE_CONFIG)) {
+|| changedTypes.contains(HelixConstants.ChangeType.RESOURCE_CONFIG)
+|| changedTypes.contains((HelixConstants.ChangeType.CLUSTER_CONFIG))) {
Review comment:
I checked. Those are deprecated usage.
Moreover, the BaseControllerDataProvider only updates for the CLUSTER_CONFIG
anyway. So it should be good for now.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #510: Fixing rebalance cache issue and stablize the tests.
jiajunwang commented on a change in pull request #510: Fixing rebalance cache
issue and stablize the tests.
URL: https://github.com/apache/helix/pull/510#discussion_r334694134
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/WagedRebalancer.java
##
@@ -216,6 +217,11 @@ public void close() {
itemKeys.addAll(getChangeDetector().getRemovalsByType(changeType));
return itemKeys;
}));
+// Filter for the items that have content changed.
+clusterChanges =
Review comment:
I tried it. But the stream method becomes too complicated. It hurts the
readability. I would keep this logic separately for easier reading. The
additional cost is low, considering the number of types is limited.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #510: Fixing rebalance cache issue and stablize the tests.
jiajunwang commented on a change in pull request #510: Fixing rebalance cache
issue and stablize the tests.
URL: https://github.com/apache/helix/pull/510#discussion_r334670299
##
File path:
helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/StrictMatchExternalViewVerifier.java
##
@@ -56,19 +55,34 @@
private final Set _resources;
private final Set _expectLiveInstances;
+ private final boolean _isDeactivatedNodeAware;
+ @Deprecated
public StrictMatchExternalViewVerifier(String zkAddr, String clusterName,
Set resources,
Set expectLiveInstances) {
+this(zkAddr, clusterName, resources, expectLiveInstances, false);
+ }
+
+ @Deprecated
+ public StrictMatchExternalViewVerifier(HelixZkClient zkClient, String
clusterName,
+ Set resources, Set expectLiveInstances) {
+this(zkClient, clusterName, resources, expectLiveInstances, false);
+ }
+
+ private StrictMatchExternalViewVerifier(String zkAddr, String clusterName,
Set resources,
+ Set expectLiveInstances, boolean isDeactivatedNodeAware) {
super(zkAddr, clusterName);
_resources = resources;
_expectLiveInstances = expectLiveInstances;
+_isDeactivatedNodeAware = isDeactivatedNodeAware;
}
- public StrictMatchExternalViewVerifier(HelixZkClient zkClient, String
clusterName,
- Set resources, Set expectLiveInstances) {
+ private StrictMatchExternalViewVerifier(HelixZkClient zkClient, String
clusterName,
Review comment:
I didn't quite get what you suggested. The current design is that this
constructor is only for builder to use.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #510: Fixing rebalance cache issue and stablize the tests.
jiajunwang commented on a change in pull request #510: Fixing rebalance cache
issue and stablize the tests.
URL: https://github.com/apache/helix/pull/510#discussion_r334670056
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/WagedRebalancer.java
##
@@ -216,6 +217,11 @@ public void close() {
itemKeys.addAll(getChangeDetector().getRemovalsByType(changeType));
return itemKeys;
}));
+// Filter for the items that have content changed.
+clusterChanges =
Review comment:
Sounds good, I will do 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #510: Fixing rebalance cache issue and stablize the tests.
jiajunwang commented on a change in pull request #510: Fixing rebalance cache
issue and stablize the tests.
URL: https://github.com/apache/helix/pull/510#discussion_r334667778
##
File path: helix-core/src/main/java/org/apache/helix/util/HelixUtil.java
##
@@ -201,23 +201,42 @@ public static String serializeByComma(List
objects) {
*/
public static Map computeIdealMapping(List
preferenceList,
StateModelDefinition stateModelDef, Set liveAndEnabled) {
+return computeIdealMapping(preferenceList, stateModelDef, liveAndEnabled,
+Collections.emptySet());
+ }
+
+ /**
+ * compute the ideal mapping for resource in Full-Auto and Semi-Auto based
on its preference list
+ */
+ public static Map computeIdealMapping(List
preferenceList,
+ StateModelDefinition stateModelDef, Set liveInstanceSet,
+ Set disabledInstancesForPartition) {
Map idealStateMap = new HashMap();
if (preferenceList == null) {
return idealStateMap;
}
+for (String instance : preferenceList) {
Review comment:
That is why we need the additional StrictMatchVerifier Builder parameter. By
default it is false. So the old behavior will be kept the same.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #510: Fixing rebalance cache issue and stablize the tests.
jiajunwang commented on a change in pull request #510: Fixing rebalance cache
issue and stablize the tests.
URL: https://github.com/apache/helix/pull/510#discussion_r334601470
##
File path:
helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/StrictMatchExternalViewVerifier.java
##
@@ -279,17 +303,21 @@ private boolean
verifyExternalView(ResourceControllerDataProvider dataCache, Ext
String stateModelDefName = idealState.getStateModelDefRef();
StateModelDefinition stateModelDef =
cache.getStateModelDef(stateModelDefName);
-Map> idealPartitionState =
-new HashMap>();
-
-Set liveEnabledInstances = new
HashSet(cache.getLiveInstances().keySet());
-liveEnabledInstances.removeAll(cache.getDisabledInstances());
+Map> idealPartitionState = new HashMap<>();
for (String partition : idealState.getPartitionSet()) {
List preferenceList = AbstractRebalancer
- .getPreferenceList(new Partition(partition), idealState,
liveEnabledInstances);
- Map idealMapping =
- HelixUtil.computeIdealMapping(preferenceList, stateModelDef,
liveEnabledInstances);
+ .getPreferenceList(new Partition(partition), idealState,
cache.getEnabledLiveInstances());
+ Map idealMapping;
+ if (_isDeactivatedNodeAware) {
+idealMapping = HelixUtil
Review comment:
The strict ev verifier uses a different method. f6f50c2 won't impact the
verifier's behavior. That's why the tests cannot pass for WAGED rebalancer even
after I merged the master.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
