[GitHub] [helix] chenboat commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable.
chenboat commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable. URL: https://github.com/apache/helix/pull/378#issuecomment-521888486 > @chenboat @narendly I'm thinking about if there is a better way to test. > > My major concern is that we make the protected method public just for testing. > Moreover, it seems we can only verify if the system property is read correctly. We cannot verify if it has been applied to the Histogram as expected. The window field is private in the metrics lib. I haven't got a good idea now : ( While most of the logic about my diff is about loading system property to init the SlidingWindow, I do understand the need to check the histogram value. Most of the current Unit tests are not aiming for testing histogram values -- they passed even I change the window length -- my understanding is that this metrics is buried pretty deep in the code. Let me take a look at some tests which could verify the histograms. 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 issue #381: Implement the POC work greedy constraint based algorithm
jiajunwang commented on issue #381: Implement the POC work greedy constraint based algorithm URL: https://github.com/apache/helix/pull/381#issuecomment-521848944 @i3wangyi As we discussed offline, please focus on the algorithm in this PR. Let's improve the model in a separate PR. In detail, please add OptimalAssignment as the output object only. During the calculation, please stick with the current design and use Cluster Model as the runtime tracking 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: 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] dasahcc commented on a change in pull request #417: Move partition health check method into dataAccessor layer
dasahcc commented on a change in pull request #417: Move partition health check
method into dataAccessor layer
URL: https://github.com/apache/helix/pull/417#discussion_r314540249
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##
@@ -234,8 +235,7 @@ private StoppableCheck performCustomInstanceCheck(String
clusterId, String insta
private Map performPartitionsCheck(List
instances,
RESTConfig restConfig, Map customPayLoads) {
-//TODO: move the heavy partition health preparation in separate class
-PartitionHealth clusterPartitionsHealth =
generatePartitionHealthMapFromZK();
+PartitionHealth clusterPartitionsHealth =
_dataAccessor.getPartitionHealthFromZK();
// update the health status for those expired partitions on instances
Map> expiredPartitionsByInstance =
Review comment:
I dont think we need many. The baseurl comes from RESTConfig. So the only
needed params are RESTConfig and list of instances.
Also you dont have to pass it in when construct the data accessor wrapper.
You can make it as function inpit in getPartitionHealth(...).
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] i3wangyi commented on a change in pull request #417: Move partition health check method into dataAccessor layer
i3wangyi commented on a change in pull request #417: Move partition health
check method into dataAccessor layer
URL: https://github.com/apache/helix/pull/417#discussion_r314536956
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##
@@ -234,8 +235,7 @@ private StoppableCheck performCustomInstanceCheck(String
clusterId, String insta
private Map performPartitionsCheck(List
instances,
RESTConfig restConfig, Map customPayLoads) {
-//TODO: move the heavy partition health preparation in separate class
-PartitionHealth clusterPartitionsHealth =
generatePartitionHealthMapFromZK();
+PartitionHealth clusterPartitionsHealth =
_dataAccessor.getPartitionHealthFromZK();
// update the health status for those expired partitions on instances
Map> expiredPartitionsByInstance =
Review comment:
yeah, I thought about it as well. Ideally yes. The problem is I need to pass
restconfig/baseurl/instances, a lot of params to the helix data accessor wrapper
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] i3wangyi commented on issue #408: Improve equals() on config objects
i3wangyi commented on issue #408: Improve equals() on config objects URL: https://github.com/apache/helix/pull/408#issuecomment-521825255 It'd be better to write some unit tests to justify - cases where original equals won't work but it should - cases with your change, the equals method works 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] narendly closed pull request #415: Add ChangeDetector interface and ResourceChangeDetector implementation
narendly closed pull request #415: Add ChangeDetector interface and ResourceChangeDetector implementation URL: https://github.com/apache/helix/pull/415 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] narendly merged pull request #418: Add ChangeDetector interface and ResourceChangeDetector implementation
narendly merged pull request #418: Add ChangeDetector interface and ResourceChangeDetector implementation URL: https://github.com/apache/helix/pull/418 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] narendly opened a new pull request #418: Add ChangeDetector interface and ResourceChangeDetector implementation
narendly opened a new pull request #418: Add ChangeDetector interface and ResourceChangeDetector implementation URL: https://github.com/apache/helix/pull/418 In order to efficiently react to changes happening to the cluster in the new WAGED rebalancer, a new component called ChangeDetector was added. Changelist: 1. Add ChangeDetector interface 1. Implement ResourceChangeDetector 1. Add ResourceChangeCache, a wrapper for critical cluster metadata 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] i3wangyi commented on issue #417: Move partition heatlh check method into dataAccessor layer
i3wangyi commented on issue #417: Move partition heatlh check method into dataAccessor layer URL: https://github.com/apache/helix/pull/417#issuecomment-521822204 The refactor is not finalized. Because partitions health from ZK is not good enough, it will need to combine the logics from rest client. However, merely passing rest client won't easily solve the problem, we will end up having a lot of params. 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] i3wangyi opened a new pull request #417: Move partition heatlh check method into dataAccessor layer
i3wangyi opened a new pull request #417: Move partition heatlh check method
into dataAccessor layer
URL: https://github.com/apache/helix/pull/417
### Issues
- [ ] My PR addresses the following Helix issues and references them in the
PR title:
fixes #416
### Description
- [ ] Here are some details about my PR, including screenshots of any UI
changes:
(Write a concise description including what, why, how)
Move the method `generateClusterPartitionHealth` method into
HelixDataAcessorWrapper, which is an ideal place to hide the complexity of ZK
related operations.
### Tests
- [ ] The following tests are written for this issue:
(List the names of added unit/integration tests)
Moved the test method in TestInstanceService to a new test class called
`TestHelixDataAccessorWrapper`
- [ ] The following is the result of the "mvn test" command on the
appropriate module:
(Copy & paste the result of "mvn test")
### Commits
- [ ] 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"
### Documentation
- [ ] In case of new functionality, my PR adds documentation in the
following wiki page:
(Link the GitHub wiki you added)
### Code Quality
- [ ] 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:
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] i3wangyi opened a new issue #416: `InstanceServiceImpl`'s getPartitionsHealth is too long and difficult to test
i3wangyi opened a new issue #416: `InstanceServiceImpl`'s getPartitionsHealth is too long and difficult to test URL: https://github.com/apache/helix/issues/416 The idea is to wrap all the heavy lifting logics (especially those for getting cluster partition health zNode) in a separate class. 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 issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable.
jiajunwang commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable. URL: https://github.com/apache/helix/pull/378#issuecomment-521818405 > @chenboat @narendly I'm thinking about if there is a better way to test. > > My major concern is that we make the protected method public just for testing. > Moreover, it seems we can only verify if the system property is read correctly. We cannot verify if it has been applied to the Histogram as expected. The window field is private in the metrics lib. I haven't got a good idea now : ( I got something. It could be a bad idea, but better than nothing. Can we add a test case that: 1. set the time window to 2 seconds. 2. create a monitor and update a histogram metric data once. Verify the expected data presents in the histogram metric. 3. Wait for 3 seconds. Check the same histogram metric data, the data should be have been reset. 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 #381: Implement the POC work greedy constraint based algorithm
jiajunwang commented on a change in pull request #381: Implement the POC work
greedy constraint based algorithm
URL: https://github.com/apache/helix/pull/381#discussion_r314508031
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/OptimalAssignment.java
##
@@ -0,0 +1,83 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.helix.controller.rebalancer.waged.constraints.HardConstraint;
+import org.apache.helix.model.ResourceAssignment;
+
+import javafx.util.Pair;
+
+/**
+ * The data model represents the optimal assignment of N replicas assigned to
M instances;
+ * It's mostly used as the return parameter of an assignment calculation
algorithm; If the algorithm
+ * failed to find
+ * optimal assignment given the endeavor, the user could check the failure
reasons
+ */
+public class OptimalAssignment {
+ private Map> _optimalAssignment =
new HashMap<>();
+ private Map, List>
_failedAssignments = new HashMap<>();
+ private ClusterContext _clusterContext;
Review comment:
As mentioned above, why not using ClusterModel here?
Or, if you prefer this style, another option would be splitting
ClusterModel. Meaning you make the current ClusterModel readonly. And move the
assign/release methods to OptimalAssignment.
I'm fine with either solution.
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 #381: Implement the POC work greedy constraint based algorithm
jiajunwang commented on a change in pull request #381: Implement the POC work
greedy constraint based algorithm
URL: https://github.com/apache/helix/pull/381#discussion_r314507807
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterContext.java
##
@@ -92,7 +96,12 @@ public int getEstimatedMaxTopStateCount() {
.getOrDefault(resourceName, Collections.emptySet());
}
- void addPartitionToFaultZone(String faultZoneId, String resourceName, String
partition) {
+ void addAssignment(AssignableNode assignableNode, AssignableReplica
assignableReplica) {
Review comment:
We have this method in the ClusterModel. Why we need it here?
I guess you want to use ClusterContext in the OptimalAssignment, but context
is designed to include only partial information. I think directly use the
ClusterModel would be easier for you.
Or you will need to add more and more interfaces like this one.
Let me clarify a little bit more, my major concern is that, ClusterContext
is not necessary to understand AssignableNode. If you merge it here, we won't
need ClusterModel.
I will agree with the change if you can merge the ClusterModel into
ClusterContext completely. Otherwise, the current design is confusing.
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 #381: Implement the POC work greedy constraint based algorithm
jiajunwang commented on a change in pull request #381: Implement the POC work
greedy constraint based algorithm
URL: https://github.com/apache/helix/pull/381#discussion_r314514578
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/algorithm/ConstraintBasedAlgorithm.java
##
@@ -0,0 +1,108 @@
+package org.apache.helix.controller.rebalancer.waged.algorithm;
+
+/*
+ * 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.Comparator;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+
+import org.apache.helix.controller.rebalancer.waged.constraints.HardConstraint;
+import org.apache.helix.controller.rebalancer.waged.constraints.SoftConstraint;
+import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
+import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
+import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
+import org.apache.helix.controller.rebalancer.waged.model.ClusterModel;
+import org.apache.helix.controller.rebalancer.waged.model.OptimalAssignment;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The algorithm is based on a given set of constraints
+ * - HardConstraint: Approve or deny the assignment given its condition, any
assignment cannot
+ * bypass any "hard constraint"
+ * - SoftConstraint: Evaluate the assignment by points/rewards/scores, a
higher point means a better
+ * assignment
+ *
+ * The goal is to accumulate the most points(rewards) from "soft constraints"
while avoiding all
+ * "hard constraints"
+ */
+public class ConstraintBasedAlgorithm implements RebalanceAlgorithm {
+ private static final Logger LOG =
LoggerFactory.getLogger(ConstraintBasedAlgorithm.class);
+ private final List _hardConstraints;
+ private final List _softConstraints;
+
+ public ConstraintBasedAlgorithm(List hardConstraints,
+ List softConstraints) {
+_hardConstraints = hardConstraints;
+_softConstraints = softConstraints;
+ }
+
+ @Override
+ public OptimalAssignment calculate(ClusterModel clusterModel) {
+ClusterContext clusterContext = clusterModel.getContext();
+OptimalAssignment optimalAssignment = new
OptimalAssignment(clusterContext);
+Map> replicasByResource =
clusterModel.getAssignableReplicaMap();
+List nodes = new
ArrayList<>(clusterModel.getAssignableNodes().values());
+
+for (String resource : replicasByResource.keySet()) {
+ for (AssignableReplica replica : replicasByResource.get(resource)) {
+Optional maybeBestNode =
+getNodeWithHighestPoints(replica, nodes, optimalAssignment);
+// stop immediately if any replica cannot find best assignable node
+if (optimalAssignment.hasAnyFailure()) {
+ LOG.error(
+ "Unable to find any available candidate node for partition {};
Fail reasons: {}",
+ replica.getPartitionName(), optimalAssignment.getFailures());
+ return optimalAssignment;
+}
+maybeBestNode.ifPresent(node ->
optimalAssignment.addAssignment(replica, node));
Review comment:
If a partition cannot be assigned completely, shall we revert the previously
proposed replicas?
For example, replica factor = 3. And minimum replica count = 2. And the
algorithm has proposed one replica assignment, but it cannot find the second
one. Shall we release the first assignment to make some room for the later
partitions?
**This is a question, I don't have the answer.**
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:
[GitHub] [helix] jiajunwang commented on a change in pull request #381: Implement the POC work greedy constraint based algorithm
jiajunwang commented on a change in pull request #381: Implement the POC work
greedy constraint based algorithm
URL: https://github.com/apache/helix/pull/381#discussion_r314508590
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/algorithm/ConstraintBasedAlgorithm.java
##
@@ -0,0 +1,108 @@
+package org.apache.helix.controller.rebalancer.waged.algorithm;
+
+/*
+ * 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.Comparator;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+
+import org.apache.helix.controller.rebalancer.waged.constraints.HardConstraint;
+import org.apache.helix.controller.rebalancer.waged.constraints.SoftConstraint;
+import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
+import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
+import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
+import org.apache.helix.controller.rebalancer.waged.model.ClusterModel;
+import org.apache.helix.controller.rebalancer.waged.model.OptimalAssignment;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The algorithm is based on a given set of constraints
+ * - HardConstraint: Approve or deny the assignment given its condition, any
assignment cannot
+ * bypass any "hard constraint"
+ * - SoftConstraint: Evaluate the assignment by points/rewards/scores, a
higher point means a better
+ * assignment
+ *
+ * The goal is to accumulate the most points(rewards) from "soft constraints"
while avoiding all
+ * "hard constraints"
+ */
+public class ConstraintBasedAlgorithm implements RebalanceAlgorithm {
+ private static final Logger LOG =
LoggerFactory.getLogger(ConstraintBasedAlgorithm.class);
+ private final List _hardConstraints;
+ private final List _softConstraints;
+
+ public ConstraintBasedAlgorithm(List hardConstraints,
+ List softConstraints) {
+_hardConstraints = hardConstraints;
+_softConstraints = softConstraints;
+ }
+
+ @Override
+ public OptimalAssignment calculate(ClusterModel clusterModel) {
+ClusterContext clusterContext = clusterModel.getContext();
+OptimalAssignment optimalAssignment = new
OptimalAssignment(clusterContext);
+Map> replicasByResource =
clusterModel.getAssignableReplicaMap();
+List nodes = new
ArrayList<>(clusterModel.getAssignableNodes().values());
+
+for (String resource : replicasByResource.keySet()) {
+ for (AssignableReplica replica : replicasByResource.get(resource)) {
+Optional maybeBestNode =
+getNodeWithHighestPoints(replica, nodes, optimalAssignment);
+// stop immediately if any replica cannot find best assignable node
+if (optimalAssignment.hasAnyFailure()) {
+ LOG.error(
+ "Unable to find any available candidate node for partition {};
Fail reasons: {}",
+ replica.getPartitionName(), optimalAssignment.getFailures());
+ return optimalAssignment;
+}
+maybeBestNode.ifPresent(node ->
optimalAssignment.addAssignment(replica, node));
+ }
+}
+
+return optimalAssignment;
+ }
+
+ private Optional getNodeWithHighestPoints(AssignableReplica
replica,
+ List assignableNodes, OptimalAssignment
currentAssignment) {
+List candidateNodes = assignableNodes.stream()
+.filter(candidateNode -> _hardConstraints.stream().allMatch(constraint
-> {
+ boolean isValid = constraint.isAssignmentValid(candidateNode,
replica, currentAssignment.getClusterContext());
+ if (!isValid) {
+currentAssignment.trackAssignmentFailure(candidateNode, replica,
constraint);
Review comment:
Is this necessary? We basically record all the possible pairs, right?
So even with a completely valid assignment, you will have (nodeCount - 1) *
replicaCount error records.
I don't think it is necessary.
This is an automated message from the Apache Git Service.
To respond to the
[GitHub] [helix] narendly opened a new pull request #415: Add ChangeDetector interface and ResourceChangeDetector implementation
narendly opened a new pull request #415: Add ChangeDetector interface and ResourceChangeDetector implementation URL: https://github.com/apache/helix/pull/415 **Copy of https://github.com/apache/helix/pull/388. It was meant to be checked into the wagedrebalancer branch. But it accidentally got merged into master branch. Creating a new PR to be merged into the wagedrebalancer branch.** In order to efficiently react to changes happening to the cluster in the new WAGED rebalancer, a new component called ChangeDetector was added. Changelist: 1. Add ChangeDetector interface 1. Implement ResourceChangeDetector 1. Add ResourceChangeCache, a wrapper for critical cluster metadata 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] narendly merged pull request #414: Revert "Add ChangeDetector interface and ResourceChangeDetector implementation"
narendly merged pull request #414: Revert "Add ChangeDetector interface and ResourceChangeDetector implementation" URL: https://github.com/apache/helix/pull/414 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] narendly opened a new pull request #414: Revert "Add ChangeDetector interface and ResourceChangeDetector implementation"
narendly opened a new pull request #414: Revert "Add ChangeDetector interface and ResourceChangeDetector implementation" URL: https://github.com/apache/helix/pull/414 Reverts apache/helix#388 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] narendly commented on issue #388: Add ChangeDetector interface and ResourceChangeDetector implementation
narendly commented on issue #388: Add ChangeDetector interface and ResourceChangeDetector implementation URL: https://github.com/apache/helix/pull/388#issuecomment-521805240 Add ChangeDetector interface and ResourceChangeDetector implementation In order to efficiently react to changes happening to the cluster in the new WAGED rebalancer, a new component called ChangeDetector was added. Changelist: Add ChangeDetector interface Implement ResourceChangeDetector Add ResourceChangeCache, a wrapper for critical cluster metadata 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] narendly commented on issue #388: Add ChangeDetector interface and ResourceChangeDetector implementation
narendly commented on issue #388: Add ChangeDetector interface and ResourceChangeDetector implementation URL: https://github.com/apache/helix/pull/388#issuecomment-521805099 This PR is ready to be merged, approved by @i3wangyi and @jiajunwang 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] i3wangyi commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
i3wangyi commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight … URL: https://github.com/apache/helix/pull/369#discussion_r314506059 ## File path: helix-core/src/test/java/org/apache/helix/controller/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java ## @@ -0,0 +1,329 @@ +package org.apache.helix.controller.strategy.crushMapping; Review comment: Just added, thanks for catching 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] narendly commented on a change in pull request #388: Add ChangeDetector interface and ResourceChangeDetector implementation
narendly commented on a change in pull request #388: Add ChangeDetector
interface and ResourceChangeDetector implementation
URL: https://github.com/apache/helix/pull/388#discussion_r314494766
##
File path:
helix-core/src/main/java/org/apache/helix/controller/changedetector/ResourceChangeDetector.java
##
@@ -0,0 +1,167 @@
+package org.apache.helix.controller.changedetector;
+
+/*
+ * 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 com.google.common.collect.Sets;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import org.apache.helix.HelixConstants;
+import org.apache.helix.HelixException;
+import org.apache.helix.HelixProperty;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+
+/**
+ * ResourceChangeDetector implements ChangeDetector. It caches
resource-related metadata from
+ * Helix's main resource pipeline cache (DataProvider) and the computation
results of change
+ * detection.
+ * WARNING: the methods of this class are not thread-safe.
+ */
+public class ResourceChangeDetector implements ChangeDetector {
+
+ private ResourceChangeSnapshot _oldSnapshot; // snapshot for previous
pipeline run
+ private ResourceChangeSnapshot _newSnapshot; // snapshot for this pipeline
run
+
+ // The following caches the computation results
+ private Map> _changedItems =
new HashMap<>();
+ private Map> _addedItems = new
HashMap<>();
+ private Map> _removedItems =
new HashMap<>();
+
+ /**
+ * Compare the underlying HelixProperty objects and produce a collection of
names of changed
+ * properties.
+ * @return
+ */
+ private Collection getChangedItems(Map oldPropertyMap,
+ Map newPropertyMap) {
+Collection changedItems = new HashSet<>();
+oldPropertyMap.forEach((name, property) -> {
+ if (newPropertyMap.containsKey(name)
+ &&
!property.getRecord().equals(newPropertyMap.get(name).getRecord())) {
+changedItems.add(name);
+ }
+});
+return changedItems;
+ }
+
+ /**
+ * Return a collection of names that are newly added.
+ * @return
+ */
+ private Collection getAddedItems(Map oldPropertyMap,
+ Map newPropertyMap) {
+return Sets.difference(newPropertyMap.keySet(), oldPropertyMap.keySet());
+ }
+
+ /**
+ * Return a collection of names that were removed.
+ * @return
+ */
+ private Collection getRemovedItems(Map oldPropertyMap,
+ Map newPropertyMap) {
+return Sets.difference(oldPropertyMap.keySet(), newPropertyMap.keySet());
+ }
+
+ private void clearCachedComputation() {
+_changedItems.clear();
+_addedItems.clear();
+_removedItems.clear();
+ }
+
+ /**
+ * Initializes old and new snapshots when ResourceChangeDetector gets its
first update.
+ */
+ private void initializeSnapshots() {
+_oldSnapshot = new ResourceChangeSnapshot();
+_newSnapshot = new ResourceChangeSnapshot();
+ }
+
+ /**
+ * Based on the change type given and propertyMap type, call the right
getters for propertyMap.
+ * @param changeType
+ * @param snapshot
+ * @return
+ */
+ private Map determinePropertyMapByType(
+ HelixConstants.ChangeType changeType, ResourceChangeSnapshot snapshot) {
+switch (changeType) {
+case INSTANCE_CONFIG:
+ return snapshot.getInstanceConfigMap();
+case IDEAL_STATE:
+ return snapshot.getIdealStateMap();
+case RESOURCE_CONFIG:
+ return snapshot.getResourceConfigMap();
+case LIVE_INSTANCE:
+ return snapshot.getLiveInstances();
+default:
+ throw new HelixException(String.format(
+ "ResourceChangeDetector cannot compute the names of changes for the
given ChangeType: %s",
+ changeType));
+}
+ }
+
+ /**
+ * Makes the current newSnapshot the oldSnapshot and reads in the up-to-date
snapshot for change
+ * computation. To be called in the controller pipeline.
+ * @param dataProvider newly refreshed DataProvider (cache)
+ */
+ public synchronized void updateSnapshots(ResourceControllerDataProvider
dataProvider) {
+// If snapshots are null, initialize them
+if (_oldSnapshot == null ||
[GitHub] [helix] i3wangyi commented on a change in pull request #381: Implement the POC work greedy constraint based algorithm
i3wangyi commented on a change in pull request #381: Implement the POC work greedy constraint based algorithm URL: https://github.com/apache/helix/pull/381#discussion_r314492924 ## File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/OptimalAssignment.java ## @@ -0,0 +1,83 @@ +package org.apache.helix.controller.rebalancer.waged.model; + +/* + * 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.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import org.apache.helix.controller.rebalancer.waged.constraints.HardConstraint; +import org.apache.helix.model.ResourceAssignment; + +import javafx.util.Pair; Review comment: Actually, I believe we need to import apache.commons library where there's a handful of classes to utilize 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] narendly merged pull request #333: Fix issue when client only sets ANY at cluster level throttle config
narendly merged pull request #333: Fix issue when client only sets ANY at cluster level throttle config URL: https://github.com/apache/helix/pull/333 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] narendly closed issue #332: Issue when client ONLY sets cluster level ANY throttle config
narendly closed issue #332: Issue when client ONLY sets cluster level ANY throttle config URL: https://github.com/apache/helix/issues/332 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 opened a new pull request #413: Add cluster level default instance config.
jiajunwang opened a new pull request #413: Add cluster level default instance
config.
URL: https://github.com/apache/helix/pull/413
### Issues
- [ ] My PR addresses the following Helix issues and references them in the
PR title:
#412
### Description
- [ ] Here are some details about my PR, including screenshots of any UI
changes:
Add cluster level default instance config.
This config will be applied to the instance when there is no (or empty)
capacity configuration in the Instance Config.
Also add unit tests.
### Tests
- [ ] The following tests are written for this issue:
TestAssignableNode
TestClusterModel
- [ ] The following is the result of the "mvn test" command on the
appropriate module:
NA since the WAGED rebalancer has not been integrated yet.
### Commits
- [ ] 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"
### Documentation
- [ ] In case of new functionality, my PR adds documentation in the
following wiki page:
https://github.com/apache/helix/wiki/Weight-aware-Globally-Evenly-distributed-Rebalancer
### Code Quality
- [ ] 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:
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 opened a new issue #412: Add a cluster level default instance capacity config for the WAGED rebalancer
jiajunwang opened a new issue #412: Add a cluster level default instance capacity config for the WAGED rebalancer URL: https://github.com/apache/helix/issues/412 During the discussion with our potential customers, we noticed that a default instance capacity could be very helpful. The default capacity will be applied to the instance when the instance level capacity map is empty or not configured. 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 #388: Add ChangeDetector interface and ResourceChangeDetector implementation
jiajunwang commented on a change in pull request #388: Add ChangeDetector
interface and ResourceChangeDetector implementation
URL: https://github.com/apache/helix/pull/388#discussion_r314452657
##
File path:
helix-core/src/main/java/org/apache/helix/controller/changedetector/ResourceChangeDetector.java
##
@@ -0,0 +1,167 @@
+package org.apache.helix.controller.changedetector;
+
+/*
+ * 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 com.google.common.collect.Sets;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import org.apache.helix.HelixConstants;
+import org.apache.helix.HelixException;
+import org.apache.helix.HelixProperty;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+
+/**
+ * ResourceChangeDetector implements ChangeDetector. It caches
resource-related metadata from
+ * Helix's main resource pipeline cache (DataProvider) and the computation
results of change
+ * detection.
+ * WARNING: the methods of this class are not thread-safe.
+ */
+public class ResourceChangeDetector implements ChangeDetector {
+
+ private ResourceChangeSnapshot _oldSnapshot; // snapshot for previous
pipeline run
+ private ResourceChangeSnapshot _newSnapshot; // snapshot for this pipeline
run
+
+ // The following caches the computation results
+ private Map> _changedItems =
new HashMap<>();
+ private Map> _addedItems = new
HashMap<>();
+ private Map> _removedItems =
new HashMap<>();
+
+ /**
+ * Compare the underlying HelixProperty objects and produce a collection of
names of changed
+ * properties.
+ * @return
+ */
+ private Collection getChangedItems(Map oldPropertyMap,
+ Map newPropertyMap) {
+Collection changedItems = new HashSet<>();
+oldPropertyMap.forEach((name, property) -> {
+ if (newPropertyMap.containsKey(name)
+ &&
!property.getRecord().equals(newPropertyMap.get(name).getRecord())) {
+changedItems.add(name);
+ }
+});
+return changedItems;
+ }
+
+ /**
+ * Return a collection of names that are newly added.
+ * @return
+ */
+ private Collection getAddedItems(Map oldPropertyMap,
+ Map newPropertyMap) {
+return Sets.difference(newPropertyMap.keySet(), oldPropertyMap.keySet());
+ }
+
+ /**
+ * Return a collection of names that were removed.
+ * @return
+ */
+ private Collection getRemovedItems(Map oldPropertyMap,
+ Map newPropertyMap) {
+return Sets.difference(oldPropertyMap.keySet(), newPropertyMap.keySet());
+ }
+
+ private void clearCachedComputation() {
+_changedItems.clear();
+_addedItems.clear();
+_removedItems.clear();
+ }
+
+ /**
+ * Initializes old and new snapshots when ResourceChangeDetector gets its
first update.
+ */
+ private void initializeSnapshots() {
+_oldSnapshot = new ResourceChangeSnapshot();
+_newSnapshot = new ResourceChangeSnapshot();
+ }
+
+ /**
+ * Based on the change type given and propertyMap type, call the right
getters for propertyMap.
+ * @param changeType
+ * @param snapshot
+ * @return
+ */
+ private Map determinePropertyMapByType(
+ HelixConstants.ChangeType changeType, ResourceChangeSnapshot snapshot) {
+switch (changeType) {
+case INSTANCE_CONFIG:
+ return snapshot.getInstanceConfigMap();
+case IDEAL_STATE:
+ return snapshot.getIdealStateMap();
+case RESOURCE_CONFIG:
+ return snapshot.getResourceConfigMap();
+case LIVE_INSTANCE:
+ return snapshot.getLiveInstances();
+default:
+ throw new HelixException(String.format(
+ "ResourceChangeDetector cannot compute the names of changes for the
given ChangeType: %s",
+ changeType));
+}
+ }
+
+ /**
+ * Makes the current newSnapshot the oldSnapshot and reads in the up-to-date
snapshot for change
+ * computation. To be called in the controller pipeline.
+ * @param dataProvider newly refreshed DataProvider (cache)
+ */
+ public synchronized void updateSnapshots(ResourceControllerDataProvider
dataProvider) {
+// If snapshots are null, initialize them
+if (_oldSnapshot == null ||
[GitHub] [helix] i3wangyi commented on issue #333: Fix issue when client only sets ANY at cluster level throttle config
i3wangyi commented on issue #333: Fix issue when client only sets ANY at cluster level throttle config URL: https://github.com/apache/helix/pull/333#issuecomment-521754515 The PR is ready to merge into master, approved by @narendly 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] i3wangyi commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config
i3wangyi commented on a change in pull request #333: Fix issue when client only
sets ANY at cluster level throttle config
URL: https://github.com/apache/helix/pull/333#discussion_r314445465
##
File path:
helix-core/src/test/java/org/apache/helix/integration/TestPartitionMovementThrottle.java
##
@@ -211,8 +217,54 @@ public void testANYtypeThrottle() throws
InterruptedException {
Thread.sleep(2000L);
for (int i = 0; i < NODE_NR; i++) {
- validateThrottle(DelayedTransition.getInstancePatitionTransitionTimes(),
- _participants[i].getInstanceName(), 1);
+
Assert.assertTrue(getMaxParallelTransitionCount(DelayedTransition.getInstancePatitionTransitionTimes(),
_participants[i].getInstanceName()) <= 1);
+}
+ }
+
+ @Test
+ public void testThrottleOnlyClusterLevelAnyType() {
+// start some participants
+for (int i = 0; i < NODE_NR - 3; i++) {
+ _participants[i].syncStart();
+}
+// Add resource: TestDB_ANY of 20 partitions
+_gSetupTool.addResourceToCluster(CLUSTER_NAME,
WorkflowGenerator.DEFAULT_TGT_DB + "_OnlyANY", 20,
+STATE_MODEL, RebalanceMode.FULL_AUTO.name());
+// Act the rebalance process
+_gSetupTool.rebalanceStorageCluster(CLUSTER_NAME,
WorkflowGenerator.DEFAULT_TGT_DB + "_OnlyANY",
+_replica);
+
+Assert.assertTrue(_clusterVerifier.verifyByPolling());
+
+// overwrite the cluster level throttle configuration
+ClusterConfig clusterConfig =
_configAccessor.getClusterConfig(CLUSTER_NAME);
+StateTransitionThrottleConfig anyTypeClusterThrottle = new
StateTransitionThrottleConfig(
+StateTransitionThrottleConfig.RebalanceType.ANY,
StateTransitionThrottleConfig.ThrottleScope.CLUSTER, 1);
+
clusterConfig.setStateTransitionThrottleConfigs(ImmutableList.of(anyTypeClusterThrottle));
+_configAccessor.setClusterConfig(CLUSTER_NAME, clusterConfig);
+
+DelayedTransition.setDelay(20);
+DelayedTransition.enableThrottleRecord();
+
+List newNodes =
Arrays.asList(_participants).subList(NODE_NR - 3, NODE_NR);
+newNodes.forEach(MockParticipantManager::syncStart);
+newNodes.forEach(node -> {
+ try {
+Assert.assertTrue(TestHelper.verify(
+() ->
getMaxParallelTransitionCount(DelayedTransition.getInstancePatitionTransitionTimes(),
node.getInstanceName()) <= 1, 1000 * 2));
+ } catch (Exception e) {
+e.printStackTrace();
+assert false;
+ }
+});
+
+ClusterLiveNodesVerifier clusterLiveNodesVerifier = new
ClusterLiveNodesVerifier(
+_gZkClient, CLUSTER_NAME,
Lists.transform(Arrays.asList(_participants),
MockParticipantManager::getInstanceName));
+Assert.assertTrue(clusterLiveNodesVerifier.verifyByZkCallback(1000));
+Assert.assertTrue(_clusterVerifier.verifyByPolling());
+
+for (int i = 0; i < NODE_NR; i++) {
+ _participants[i].syncStop();
Review comment:
Sounds good. Added the verifier in the @aftermethod so every test needs to
check if the cleanup work is finished.
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] i3wangyi commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config
i3wangyi commented on a change in pull request #333: Fix issue when client only
sets ANY at cluster level throttle config
URL: https://github.com/apache/helix/pull/333#discussion_r314439745
##
File path:
helix-core/src/test/java/org/apache/helix/controller/stages/TestStateTransitionThrottleController.java
##
@@ -0,0 +1,193 @@
+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 static
org.apache.helix.api.config.StateTransitionThrottleConfig.RebalanceType.ANY;
+import static
org.apache.helix.api.config.StateTransitionThrottleConfig.RebalanceType.LOAD_BALANCE;
+import static
org.apache.helix.api.config.StateTransitionThrottleConfig.RebalanceType.RECOVERY_BALANCE;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import org.apache.helix.api.config.StateTransitionThrottleConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.testng.Assert;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+
+public class TestStateTransitionThrottleController {
+ private static final String INSTANCE = "instance0";
+ private static final String RESOURCE = "db0";
+ private static final List
VALID_REBALANCE_TYPES =
+ ImmutableList.of(LOAD_BALANCE, RECOVERY_BALANCE, ANY);
+
+ @Test(description = "When cluster level ANY throttle config is set")
+ public void testChargeClusterWhenANYClusterLevelThrottleConfig() {
+int maxNumberOfST = 1;
+ClusterConfig clusterConfig = new ClusterConfig("config");
+clusterConfig
+.setStateTransitionThrottleConfigs(ImmutableList.of(new
StateTransitionThrottleConfig(ANY,
+StateTransitionThrottleConfig.ThrottleScope.CLUSTER,
maxNumberOfST)));
+
+StateTransitionThrottleControllerAccessor controller =
+new StateTransitionThrottleControllerAccessor(RESOURCE, INSTANCE,
clusterConfig);
+Assert.assertTrue(controller.isThrottleEnabled());
+
+for (StateTransitionThrottleConfig.RebalanceType rebalanceType :
VALID_REBALANCE_TYPES) {
+ controller.chargeCluster(rebalanceType);
+ for (StateTransitionThrottleConfig.RebalanceType type :
VALID_REBALANCE_TYPES) {
+Assert.assertTrue(controller.shouldThrottleForCluster(type));
+Assert.assertTrue(controller.shouldThrottleForInstance(type,
INSTANCE));
+Assert.assertTrue(controller.shouldThrottleForInstance(type,
RESOURCE));
+ }
+ // reset controller
+ controller = new StateTransitionThrottleControllerAccessor(RESOURCE,
INSTANCE, clusterConfig);
+}
+ }
+
+ @Test(description = "When cluster throttle is config of
LOAD_BALANCE/RECOVERY_BALANCE, no ANY type")
+ public void testChargeCluster_OnlySetClusterSpecificType() {
+int maxNumberOfST = 1;
+ClusterConfig clusterConfig = new ClusterConfig("config");
+clusterConfig.setStateTransitionThrottleConfigs(ImmutableList.of(
+new StateTransitionThrottleConfig(RECOVERY_BALANCE,
+StateTransitionThrottleConfig.ThrottleScope.CLUSTER,
maxNumberOfST),
+new StateTransitionThrottleConfig(LOAD_BALANCE,
+StateTransitionThrottleConfig.ThrottleScope.CLUSTER,
maxNumberOfST)));
+
+StateTransitionThrottleControllerAccessor controller =
+new StateTransitionThrottleControllerAccessor(RESOURCE, INSTANCE,
clusterConfig);
+
+Assert.assertTrue(controller.isThrottleEnabled());
+
+controller.chargeCluster(ANY);
+Assert.assertEquals(controller.getClusterLevelQuota(RECOVERY_BALANCE), 1);
+Assert.assertEquals(controller.getClusterLevelQuota(LOAD_BALANCE), 1);
+Assert.assertEquals(controller.getClusterLevelQuota(ANY), 0);
+
+VALID_REBALANCE_TYPES.forEach(controller::chargeCluster);
+for (StateTransitionThrottleConfig.RebalanceType rebalanceType :
ImmutableList.of(LOAD_BALANCE,
+RECOVERY_BALANCE)) {
+ Assert.assertTrue(controller.shouldThrottleForCluster(rebalanceType));
+ Assert.assertTrue(controller.shouldThrottleForInstance(rebalanceType,
INSTANCE));
+
[GitHub] [helix] alirezazamani commented on a change in pull request #410: Fix the execution delay for the jobs (409)
alirezazamani commented on a change in pull request #410: Fix the execution
delay for the jobs (409)
URL: https://github.com/apache/helix/pull/410#discussion_r314388288
##
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestExecutionDelay.java
##
@@ -0,0 +1,79 @@
+package org.apache.helix.integration.task;
+
+/*
+ * 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 com.google.common.collect.ImmutableMap;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.TestHelper;
+import org.apache.helix.task.JobConfig;
+import org.apache.helix.task.JobQueue;
+import org.apache.helix.task.TaskState;
+import org.apache.helix.task.TaskUtil;
+import org.apache.helix.task.Workflow;
+import org.apache.helix.task.WorkflowContext;
+import
org.apache.helix.tools.ClusterVerifiers.BestPossibleExternalViewVerifier;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+public class TestExecutionDelay extends TaskTestBase {
+ private HelixAdmin admin;
Review comment:
Not used. I deleted it in new commit.
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] alirezazamani commented on a change in pull request #410: Fix the execution delay for the jobs (409)
alirezazamani commented on a change in pull request #410: Fix the execution
delay for the jobs (409)
URL: https://github.com/apache/helix/pull/410#discussion_r314388288
##
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestExecutionDelay.java
##
@@ -0,0 +1,79 @@
+package org.apache.helix.integration.task;
+
+/*
+ * 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 com.google.common.collect.ImmutableMap;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.TestHelper;
+import org.apache.helix.task.JobConfig;
+import org.apache.helix.task.JobQueue;
+import org.apache.helix.task.TaskState;
+import org.apache.helix.task.TaskUtil;
+import org.apache.helix.task.Workflow;
+import org.apache.helix.task.WorkflowContext;
+import
org.apache.helix.tools.ClusterVerifiers.BestPossibleExternalViewVerifier;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+public class TestExecutionDelay extends TaskTestBase {
+ private HelixAdmin admin;
Review comment:
Not used. I deleted 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] alirezazamani commented on a change in pull request #410: Fix the execution delay for the jobs (409)
alirezazamani commented on a change in pull request #410: Fix the execution
delay for the jobs (409)
URL: https://github.com/apache/helix/pull/410#discussion_r314387956
##
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestExecutionDelay.java
##
@@ -0,0 +1,79 @@
+package org.apache.helix.integration.task;
+
+/*
+ * 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 com.google.common.collect.ImmutableMap;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.TestHelper;
+import org.apache.helix.task.JobConfig;
+import org.apache.helix.task.JobQueue;
+import org.apache.helix.task.TaskState;
+import org.apache.helix.task.TaskUtil;
+import org.apache.helix.task.Workflow;
+import org.apache.helix.task.WorkflowContext;
+import
org.apache.helix.tools.ClusterVerifiers.BestPossibleExternalViewVerifier;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+public class TestExecutionDelay extends TaskTestBase {
+ private HelixAdmin admin;
+
+ @BeforeClass public void beforeClass() throws Exception {
+admin = _gSetupTool.getClusterManagementTool();
+super.beforeClass();
+ }
+
+ @Test(expectedExceptions = HelixException.class) public void
testExecutionDelay()
Review comment:
Fixed.
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] alirezazamani commented on a change in pull request #410: Fix the execution delay for the jobs (409)
alirezazamani commented on a change in pull request #410: Fix the execution
delay for the jobs (409)
URL: https://github.com/apache/helix/pull/410#discussion_r314386144
##
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestExecutionDelay.java
##
@@ -0,0 +1,79 @@
+package org.apache.helix.integration.task;
+
+/*
+ * 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 com.google.common.collect.ImmutableMap;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.TestHelper;
+import org.apache.helix.task.JobConfig;
+import org.apache.helix.task.JobQueue;
+import org.apache.helix.task.TaskState;
+import org.apache.helix.task.TaskUtil;
+import org.apache.helix.task.Workflow;
+import org.apache.helix.task.WorkflowContext;
+import
org.apache.helix.tools.ClusterVerifiers.BestPossibleExternalViewVerifier;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+public class TestExecutionDelay extends TaskTestBase {
+ private HelixAdmin admin;
+
+ @BeforeClass public void beforeClass() throws Exception {
+admin = _gSetupTool.getClusterManagementTool();
+super.beforeClass();
Review comment:
I fixed that. Thanks.
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] alirezazamani commented on a change in pull request #410: Fix the execution delay for the jobs (409)
alirezazamani commented on a change in pull request #410: Fix the execution
delay for the jobs (409)
URL: https://github.com/apache/helix/pull/410#discussion_r31438
##
File path:
helix-core/src/main/java/org/apache/helix/task/WorkflowDispatcher.java
##
@@ -165,7 +165,9 @@ public void updateWorkflowStatus(String workflow,
WorkflowConfig workflowCfg,
RuntimeJobDag runtimeJobDag =
_clusterDataCache.getTaskDataCache().getRuntimeJobDag(workflow);
if (runtimeJobDag != null) {
for (String inflightJob : runtimeJobDag.getInflightJobList()) {
-processJob(inflightJob, currentStateOutput, bestPossibleOutput,
workflowCtx);
+if (System.currentTimeMillis() >=
workflowCtx.getJobStartTime(inflightJob)) {
Review comment:
Thank you for your comments.
1- Based on same logic that is added few lines below (which calculates the
start time and store these information and process the job if possible), this
is the place that this check should happen. (same as the scheduleJobs method in
WorkflowDistpatcher).
2- About the comprehensive change, I agree with you. This is just a quick
fix. We will need to revisit several concepts such as inflight jobs and retry
delay. I am working on the retry delay and will create new issue for that as
well. After that we can think on a comprehensive changes to the Task Framework.
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] alirezazamani commented on a change in pull request #410: Fix the execution delay for the jobs (409)
alirezazamani commented on a change in pull request #410: Fix the execution
delay for the jobs (409)
URL: https://github.com/apache/helix/pull/410#discussion_r31438
##
File path:
helix-core/src/main/java/org/apache/helix/task/WorkflowDispatcher.java
##
@@ -165,7 +165,9 @@ public void updateWorkflowStatus(String workflow,
WorkflowConfig workflowCfg,
RuntimeJobDag runtimeJobDag =
_clusterDataCache.getTaskDataCache().getRuntimeJobDag(workflow);
if (runtimeJobDag != null) {
for (String inflightJob : runtimeJobDag.getInflightJobList()) {
-processJob(inflightJob, currentStateOutput, bestPossibleOutput,
workflowCtx);
+if (System.currentTimeMillis() >=
workflowCtx.getJobStartTime(inflightJob)) {
Review comment:
Thank you for your comments.
1- Based on same logic that is added few line below (which calculates the
start time and store these information and process the job if possible), this
is the place that this check should happen. (same as the scheduleJobs method in
WorkflowDistpatcher).
2- About the comprehensive change, I agree with you. This is just a quick
fix. We will need to revisit several concepts such as inflight jobs and retry
delay. I am working on the retry delay and will create new issue for that as
well. After that we can think on a comprehensive changes to the Task Framework.
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] narendly commented on a change in pull request #388: Add ChangeDetector interface and ResourceChangeDetector implementation
narendly commented on a change in pull request #388: Add ChangeDetector
interface and ResourceChangeDetector implementation
URL: https://github.com/apache/helix/pull/388#discussion_r314375719
##
File path:
helix-core/src/main/java/org/apache/helix/controller/changedetector/ResourceChangeDetector.java
##
@@ -0,0 +1,167 @@
+package org.apache.helix.controller.changedetector;
+
+/*
+ * 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 com.google.common.collect.Sets;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import org.apache.helix.HelixConstants;
+import org.apache.helix.HelixException;
+import org.apache.helix.HelixProperty;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+
+/**
+ * ResourceChangeDetector implements ChangeDetector. It caches
resource-related metadata from
+ * Helix's main resource pipeline cache (DataProvider) and the computation
results of change
+ * detection.
+ * WARNING: the methods of this class are not thread-safe.
+ */
+public class ResourceChangeDetector implements ChangeDetector {
+
+ private ResourceChangeSnapshot _oldSnapshot; // snapshot for previous
pipeline run
+ private ResourceChangeSnapshot _newSnapshot; // snapshot for this pipeline
run
+
+ // The following caches the computation results
+ private Map> _changedItems =
new HashMap<>();
+ private Map> _addedItems = new
HashMap<>();
+ private Map> _removedItems =
new HashMap<>();
+
+ /**
+ * Compare the underlying HelixProperty objects and produce a collection of
names of changed
+ * properties.
+ * @return
+ */
+ private Collection getChangedItems(Map oldPropertyMap,
+ Map newPropertyMap) {
+Collection changedItems = new HashSet<>();
+oldPropertyMap.forEach((name, property) -> {
+ if (newPropertyMap.containsKey(name)
+ &&
!property.getRecord().equals(newPropertyMap.get(name).getRecord())) {
+changedItems.add(name);
+ }
+});
+return changedItems;
+ }
+
+ /**
+ * Return a collection of names that are newly added.
+ * @return
+ */
+ private Collection getAddedItems(Map oldPropertyMap,
+ Map newPropertyMap) {
+return Sets.difference(newPropertyMap.keySet(), oldPropertyMap.keySet());
+ }
+
+ /**
+ * Return a collection of names that were removed.
+ * @return
+ */
+ private Collection getRemovedItems(Map oldPropertyMap,
+ Map newPropertyMap) {
+return Sets.difference(oldPropertyMap.keySet(), newPropertyMap.keySet());
+ }
+
+ private void clearCachedComputation() {
+_changedItems.clear();
+_addedItems.clear();
+_removedItems.clear();
+ }
+
+ /**
+ * Initializes old and new snapshots when ResourceChangeDetector gets its
first update.
+ */
+ private void initializeSnapshots() {
+_oldSnapshot = new ResourceChangeSnapshot();
+_newSnapshot = new ResourceChangeSnapshot();
+ }
+
+ /**
+ * Based on the change type given and propertyMap type, call the right
getters for propertyMap.
+ * @param changeType
+ * @param snapshot
+ * @return
+ */
+ private Map determinePropertyMapByType(
+ HelixConstants.ChangeType changeType, ResourceChangeSnapshot snapshot) {
+switch (changeType) {
+case INSTANCE_CONFIG:
+ return snapshot.getInstanceConfigMap();
+case IDEAL_STATE:
+ return snapshot.getIdealStateMap();
+case RESOURCE_CONFIG:
+ return snapshot.getResourceConfigMap();
+case LIVE_INSTANCE:
+ return snapshot.getLiveInstances();
+default:
+ throw new HelixException(String.format(
+ "ResourceChangeDetector cannot compute the names of changes for the
given ChangeType: %s",
+ changeType));
+}
+ }
+
+ /**
+ * Makes the current newSnapshot the oldSnapshot and reads in the up-to-date
snapshot for change
+ * computation. To be called in the controller pipeline.
+ * @param dataProvider newly refreshed DataProvider (cache)
+ */
+ public synchronized void updateSnapshots(ResourceControllerDataProvider
dataProvider) {
+// If snapshots are null, initialize them
+if (_oldSnapshot == null ||
[GitHub] [helix] narendly commented on a change in pull request #388: Add ChangeDetector interface and ResourceChangeDetector implementation
narendly commented on a change in pull request #388: Add ChangeDetector
interface and ResourceChangeDetector implementation
URL: https://github.com/apache/helix/pull/388#discussion_r314374533
##
File path:
helix-core/src/main/java/org/apache/helix/controller/changedetector/ResourceChangeDetector.java
##
@@ -0,0 +1,167 @@
+package org.apache.helix.controller.changedetector;
+
+/*
+ * 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 com.google.common.collect.Sets;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import org.apache.helix.HelixConstants;
+import org.apache.helix.HelixException;
+import org.apache.helix.HelixProperty;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+
+/**
+ * ResourceChangeDetector implements ChangeDetector. It caches
resource-related metadata from
+ * Helix's main resource pipeline cache (DataProvider) and the computation
results of change
+ * detection.
+ * WARNING: the methods of this class are not thread-safe.
+ */
+public class ResourceChangeDetector implements ChangeDetector {
+
+ private ResourceChangeSnapshot _oldSnapshot; // snapshot for previous
pipeline run
+ private ResourceChangeSnapshot _newSnapshot; // snapshot for this pipeline
run
+
+ // The following caches the computation results
+ private Map> _changedItems =
new HashMap<>();
+ private Map> _addedItems = new
HashMap<>();
+ private Map> _removedItems =
new HashMap<>();
+
+ /**
+ * Compare the underlying HelixProperty objects and produce a collection of
names of changed
+ * properties.
+ * @return
+ */
+ private Collection getChangedItems(Map oldPropertyMap,
+ Map newPropertyMap) {
+Collection changedItems = new HashSet<>();
+oldPropertyMap.forEach((name, property) -> {
+ if (newPropertyMap.containsKey(name)
+ &&
!property.getRecord().equals(newPropertyMap.get(name).getRecord())) {
+changedItems.add(name);
+ }
+});
+return changedItems;
+ }
+
+ /**
+ * Return a collection of names that are newly added.
+ * @return
+ */
+ private Collection getAddedItems(Map oldPropertyMap,
+ Map newPropertyMap) {
+return Sets.difference(newPropertyMap.keySet(), oldPropertyMap.keySet());
+ }
+
+ /**
+ * Return a collection of names that were removed.
+ * @return
+ */
+ private Collection getRemovedItems(Map oldPropertyMap,
+ Map newPropertyMap) {
+return Sets.difference(oldPropertyMap.keySet(), newPropertyMap.keySet());
+ }
+
+ private void clearCachedComputation() {
+_changedItems.clear();
+_addedItems.clear();
+_removedItems.clear();
+ }
+
+ /**
+ * Initializes old and new snapshots when ResourceChangeDetector gets its
first update.
+ */
+ private void initializeSnapshots() {
+_oldSnapshot = new ResourceChangeSnapshot();
+_newSnapshot = new ResourceChangeSnapshot();
+ }
+
+ /**
+ * Based on the change type given and propertyMap type, call the right
getters for propertyMap.
+ * @param changeType
+ * @param snapshot
+ * @return
+ */
+ private Map determinePropertyMapByType(
+ HelixConstants.ChangeType changeType, ResourceChangeSnapshot snapshot) {
+switch (changeType) {
+case INSTANCE_CONFIG:
+ return snapshot.getInstanceConfigMap();
+case IDEAL_STATE:
+ return snapshot.getIdealStateMap();
+case RESOURCE_CONFIG:
+ return snapshot.getResourceConfigMap();
+case LIVE_INSTANCE:
+ return snapshot.getLiveInstances();
+default:
+ throw new HelixException(String.format(
+ "ResourceChangeDetector cannot compute the names of changes for the
given ChangeType: %s",
+ changeType));
+}
+ }
+
+ /**
+ * Makes the current newSnapshot the oldSnapshot and reads in the up-to-date
snapshot for change
+ * computation. To be called in the controller pipeline.
+ * @param dataProvider newly refreshed DataProvider (cache)
+ */
+ public synchronized void updateSnapshots(ResourceControllerDataProvider
dataProvider) {
+// If snapshots are null, initialize them
+if (_oldSnapshot == null ||
[GitHub] [helix] jiajunwang commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable.
jiajunwang commented on issue #378: Make the reservoir used in ZkClientPathMonitor metric configurable. URL: https://github.com/apache/helix/pull/378#issuecomment-521530508 @chenboat @narendly I'm thinking about if there is a better way to test. My major concern is that we make the protected method public just for testing. Moreover, it seems we can only verify if the system property is read correctly. We cannot verify if it has been applied to the Histogram as expected. The window field is private in the metrics lib. I haven't got a good idea 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 #388: Add ChangeDetector interface and ResourceChangeDetector implementation
jiajunwang commented on a change in pull request #388: Add ChangeDetector
interface and ResourceChangeDetector implementation
URL: https://github.com/apache/helix/pull/388#discussion_r314185684
##
File path:
helix-core/src/main/java/org/apache/helix/controller/changedetector/ResourceChangeDetector.java
##
@@ -0,0 +1,167 @@
+package org.apache.helix.controller.changedetector;
+
+/*
+ * 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 com.google.common.collect.Sets;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import org.apache.helix.HelixConstants;
+import org.apache.helix.HelixException;
+import org.apache.helix.HelixProperty;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+
+/**
+ * ResourceChangeDetector implements ChangeDetector. It caches
resource-related metadata from
+ * Helix's main resource pipeline cache (DataProvider) and the computation
results of change
+ * detection.
+ * WARNING: the methods of this class are not thread-safe.
+ */
+public class ResourceChangeDetector implements ChangeDetector {
+
+ private ResourceChangeSnapshot _oldSnapshot; // snapshot for previous
pipeline run
+ private ResourceChangeSnapshot _newSnapshot; // snapshot for this pipeline
run
+
+ // The following caches the computation results
+ private Map> _changedItems =
new HashMap<>();
+ private Map> _addedItems = new
HashMap<>();
+ private Map> _removedItems =
new HashMap<>();
+
+ /**
+ * Compare the underlying HelixProperty objects and produce a collection of
names of changed
+ * properties.
+ * @return
+ */
+ private Collection getChangedItems(Map oldPropertyMap,
+ Map newPropertyMap) {
+Collection changedItems = new HashSet<>();
+oldPropertyMap.forEach((name, property) -> {
+ if (newPropertyMap.containsKey(name)
+ &&
!property.getRecord().equals(newPropertyMap.get(name).getRecord())) {
+changedItems.add(name);
+ }
+});
+return changedItems;
+ }
+
+ /**
+ * Return a collection of names that are newly added.
+ * @return
+ */
+ private Collection getAddedItems(Map oldPropertyMap,
+ Map newPropertyMap) {
+return Sets.difference(newPropertyMap.keySet(), oldPropertyMap.keySet());
+ }
+
+ /**
+ * Return a collection of names that were removed.
+ * @return
+ */
+ private Collection getRemovedItems(Map oldPropertyMap,
+ Map newPropertyMap) {
+return Sets.difference(oldPropertyMap.keySet(), newPropertyMap.keySet());
+ }
+
+ private void clearCachedComputation() {
+_changedItems.clear();
+_addedItems.clear();
+_removedItems.clear();
+ }
+
+ /**
+ * Initializes old and new snapshots when ResourceChangeDetector gets its
first update.
+ */
+ private void initializeSnapshots() {
+_oldSnapshot = new ResourceChangeSnapshot();
+_newSnapshot = new ResourceChangeSnapshot();
+ }
+
+ /**
+ * Based on the change type given and propertyMap type, call the right
getters for propertyMap.
+ * @param changeType
+ * @param snapshot
+ * @return
+ */
+ private Map determinePropertyMapByType(
+ HelixConstants.ChangeType changeType, ResourceChangeSnapshot snapshot) {
+switch (changeType) {
+case INSTANCE_CONFIG:
+ return snapshot.getInstanceConfigMap();
+case IDEAL_STATE:
+ return snapshot.getIdealStateMap();
+case RESOURCE_CONFIG:
+ return snapshot.getResourceConfigMap();
+case LIVE_INSTANCE:
+ return snapshot.getLiveInstances();
+default:
+ throw new HelixException(String.format(
+ "ResourceChangeDetector cannot compute the names of changes for the
given ChangeType: %s",
+ changeType));
+}
+ }
+
+ /**
+ * Makes the current newSnapshot the oldSnapshot and reads in the up-to-date
snapshot for change
+ * computation. To be called in the controller pipeline.
+ * @param dataProvider newly refreshed DataProvider (cache)
+ */
+ public synchronized void updateSnapshots(ResourceControllerDataProvider
dataProvider) {
+// If snapshots are null, initialize them
+if (_oldSnapshot == null ||
[GitHub] [helix] jiajunwang commented on a change in pull request #388: Add ChangeDetector interface and ResourceChangeDetector implementation
jiajunwang commented on a change in pull request #388: Add ChangeDetector
interface and ResourceChangeDetector implementation
URL: https://github.com/apache/helix/pull/388#discussion_r314186025
##
File path:
helix-core/src/main/java/org/apache/helix/controller/changedetector/ResourceChangeDetector.java
##
@@ -0,0 +1,167 @@
+package org.apache.helix.controller.changedetector;
+
+/*
+ * 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 com.google.common.collect.Sets;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import org.apache.helix.HelixConstants;
+import org.apache.helix.HelixException;
+import org.apache.helix.HelixProperty;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+
+/**
+ * ResourceChangeDetector implements ChangeDetector. It caches
resource-related metadata from
+ * Helix's main resource pipeline cache (DataProvider) and the computation
results of change
+ * detection.
+ * WARNING: the methods of this class are not thread-safe.
+ */
+public class ResourceChangeDetector implements ChangeDetector {
+
+ private ResourceChangeSnapshot _oldSnapshot; // snapshot for previous
pipeline run
+ private ResourceChangeSnapshot _newSnapshot; // snapshot for this pipeline
run
+
+ // The following caches the computation results
+ private Map> _changedItems =
new HashMap<>();
+ private Map> _addedItems = new
HashMap<>();
+ private Map> _removedItems =
new HashMap<>();
+
+ /**
+ * Compare the underlying HelixProperty objects and produce a collection of
names of changed
+ * properties.
+ * @return
+ */
+ private Collection getChangedItems(Map oldPropertyMap,
+ Map newPropertyMap) {
+Collection changedItems = new HashSet<>();
+oldPropertyMap.forEach((name, property) -> {
+ if (newPropertyMap.containsKey(name)
+ &&
!property.getRecord().equals(newPropertyMap.get(name).getRecord())) {
+changedItems.add(name);
+ }
+});
+return changedItems;
+ }
+
+ /**
+ * Return a collection of names that are newly added.
+ * @return
+ */
+ private Collection getAddedItems(Map oldPropertyMap,
+ Map newPropertyMap) {
+return Sets.difference(newPropertyMap.keySet(), oldPropertyMap.keySet());
+ }
+
+ /**
+ * Return a collection of names that were removed.
+ * @return
+ */
+ private Collection getRemovedItems(Map oldPropertyMap,
+ Map newPropertyMap) {
+return Sets.difference(oldPropertyMap.keySet(), newPropertyMap.keySet());
+ }
+
+ private void clearCachedComputation() {
+_changedItems.clear();
+_addedItems.clear();
+_removedItems.clear();
+ }
+
+ /**
+ * Initializes old and new snapshots when ResourceChangeDetector gets its
first update.
+ */
+ private void initializeSnapshots() {
+_oldSnapshot = new ResourceChangeSnapshot();
+_newSnapshot = new ResourceChangeSnapshot();
+ }
+
+ /**
+ * Based on the change type given and propertyMap type, call the right
getters for propertyMap.
+ * @param changeType
+ * @param snapshot
+ * @return
+ */
+ private Map determinePropertyMapByType(
+ HelixConstants.ChangeType changeType, ResourceChangeSnapshot snapshot) {
+switch (changeType) {
+case INSTANCE_CONFIG:
+ return snapshot.getInstanceConfigMap();
+case IDEAL_STATE:
+ return snapshot.getIdealStateMap();
+case RESOURCE_CONFIG:
+ return snapshot.getResourceConfigMap();
+case LIVE_INSTANCE:
+ return snapshot.getLiveInstances();
+default:
+ throw new HelixException(String.format(
+ "ResourceChangeDetector cannot compute the names of changes for the
given ChangeType: %s",
+ changeType));
+}
+ }
+
+ /**
+ * Makes the current newSnapshot the oldSnapshot and reads in the up-to-date
snapshot for change
+ * computation. To be called in the controller pipeline.
+ * @param dataProvider newly refreshed DataProvider (cache)
+ */
+ public synchronized void updateSnapshots(ResourceControllerDataProvider
dataProvider) {
+// If snapshots are null, initialize them
+if (_oldSnapshot == null ||
