[GitHub] [helix] narendly closed pull request #423: Assignment Metadata Store
narendly closed pull request #423: Assignment Metadata Store URL: https://github.com/apache/helix/pull/423 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 #423: Assignment Metadata Store
narendly opened a new pull request #423: Assignment Metadata Store URL: https://github.com/apache/helix/pull/423 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] 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-522853075 This PR is ready to be merged, approved by [@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] 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-522852156 > Please try to run "mvn test". I ran the mvn for helix-core. There are transient failures which can pass after re-run. The mbean tests are all good. 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_r315478375
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##
@@ -1,24 +1,144 @@
package org.apache.helix.rest.common;
-import org.apache.helix.HelixProperty;
-import org.apache.helix.PropertyKey;
-import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+/*
+ * 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.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.stream.Collectors;
+
+import org.apache.helix.HelixProperty;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+import org.apache.helix.model.RESTConfig;
+import org.apache.helix.rest.client.CustomRestClient;
+import org.apache.helix.rest.client.CustomRestClientFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* This is a wrapper for {@link ZKHelixDataAccessor} that caches the result of
the batch reads it
* performs.
* Note that the usage of this object is valid for one REST request.
*/
public class HelixDataAccessorWrapper extends ZKHelixDataAccessor {
+ private static final Logger LOG =
LoggerFactory.getLogger(HelixDataAccessorWrapper.class);
+ private static final ExecutorService POOL = Executors.newCachedThreadPool();
+
+ static final String PARTITION_HEALTH_KEY = "PARTITION_HEALTH";
+ static final String IS_HEALTHY_KEY = "IS_HEALTHY";
+ static final String EXPIRY_KEY = "EXPIRE";
+
private final Map _propertyCache = new
HashMap<>();
private final Map> _batchNameCache = new
HashMap<>();
+ protected CustomRestClient _restClient;
public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor) {
super(dataAccessor);
+_restClient = CustomRestClientFactory.get();
+ }
+
+ public Map>
getAllPartitionsHealthOnLiveInstance(RESTConfig restConfig,
+
Map customPayLoads) {
+// Only checks the instances are online with valid reports
+List liveInstances = getChildNames(keyBuilder().liveInstances());
+// Make a parallel batch call for getting all healthreports from ZK.
+List zkHealthReports = getProperty(liveInstances.stream()
+.map(instance -> keyBuilder().healthReport(instance,
PARTITION_HEALTH_KEY))
+.collect(Collectors.toList()), false);
+Map>> parallelTasks = new HashMap<>();
+for (int i = 0; i < liveInstances.size(); i++) {
+ String liveInstance = liveInstances.get(i);
+ Optional maybeHealthRecord = zkHealthReports != null &&
zkHealthReports.size() > i
Review comment:
'getProperty' method was not a very good design but it's already there. I
may have done redundant checks to be extra safe, it won't break the
functionality. If I'm asking 6 instances' records, it returns {null, record1,
null, null, record, null}, it will also work.
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_r315476495
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##
@@ -1,24 +1,144 @@
package org.apache.helix.rest.common;
-import org.apache.helix.HelixProperty;
-import org.apache.helix.PropertyKey;
-import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+/*
+ * 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.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.stream.Collectors;
+
+import org.apache.helix.HelixProperty;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+import org.apache.helix.model.RESTConfig;
+import org.apache.helix.rest.client.CustomRestClient;
+import org.apache.helix.rest.client.CustomRestClientFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* This is a wrapper for {@link ZKHelixDataAccessor} that caches the result of
the batch reads it
* performs.
* Note that the usage of this object is valid for one REST request.
*/
public class HelixDataAccessorWrapper extends ZKHelixDataAccessor {
+ private static final Logger LOG =
LoggerFactory.getLogger(HelixDataAccessorWrapper.class);
+ private static final ExecutorService POOL = Executors.newCachedThreadPool();
+
+ static final String PARTITION_HEALTH_KEY = "PARTITION_HEALTH";
+ static final String IS_HEALTHY_KEY = "IS_HEALTHY";
+ static final String EXPIRY_KEY = "EXPIRE";
+
private final Map _propertyCache = new
HashMap<>();
private final Map> _batchNameCache = new
HashMap<>();
+ protected CustomRestClient _restClient;
public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor) {
super(dataAccessor);
+_restClient = CustomRestClientFactory.get();
+ }
+
+ public Map>
getAllPartitionsHealthOnLiveInstance(RESTConfig restConfig,
+
Map customPayLoads) {
+// Only checks the instances are online with valid reports
+List liveInstances = getChildNames(keyBuilder().liveInstances());
+// Make a parallel batch call for getting all healthreports from ZK.
+List zkHealthReports = getProperty(liveInstances.stream()
+.map(instance -> keyBuilder().healthReport(instance,
PARTITION_HEALTH_KEY))
+.collect(Collectors.toList()), false);
+Map>> parallelTasks = new HashMap<>();
+for (int i = 0; i < liveInstances.size(); i++) {
+ String liveInstance = liveInstances.get(i);
+ Optional maybeHealthRecord = zkHealthReports != null &&
zkHealthReports.size() > i
Review comment:
If Helix does not return you complete list, this is not an efficient way.
You dont know which one is missed from the array list. Then you totally mess up
the result. For example, you have 6 instances and you only get 3 ZNRecords for
last 3 ones. With your logic, you will fill the 3 ZNRecords as first threes
health report and querying next three ones.
Please refer the Java doc of get in BaseDataAccessor interface. getProperty
uses the get() to return list of HelixProperty:
https://github.com/apache/helix/blob/master/helix-core/src/main/java/org/apache/helix/BaseDataAccessor.java#L150
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
[GitHub] [helix] dasahcc commented on a change in pull request #422: Redefine the hard/soft constraints
dasahcc commented on a change in pull request #422: Redefine the hard/soft
constraints
URL: https://github.com/apache/helix/pull/422#discussion_r315475010
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/SoftConstraintWeightModel.java
##
@@ -0,0 +1,47 @@
+package org.apache.helix.controller.rebalancer.waged.constraints;
+
+import java.util.Map;
+
+import com.google.common.collect.ImmutableMap;
+
+/**
+ * The class retrieves the offline model that defines the relative importance
of soft constraints.
+ */
+class SoftConstraintWeightModel {
+ private float MIN_SCORE = Float.MIN_VALUE;
+ private float MAX_SCORE = Float.MAX_VALUE;
+ private static Map MODEL;
+
+ static {
+// TODO either define the weights in property files or zookeeper node or
static human input
+MODEL = ImmutableMap. builder()
+.put(SoftConstraint.Type.LEAST_MOVEMENTS, 1.0f)
+.put(SoftConstraint.Type.LEAST_PARTITION_COUNT, 1.0f)
+.put(SoftConstraint.Type.LEAST_USED_NODE, 1.0f).build();
+ }
+
+ interface ScoreScaler {
+/**
+ * Method to scale the origin score to a normalized range
+ * @param originScore The origin score of a range
+ * @return The normalized value between 0 - 1
+ */
+float scale(float originScore);
+ }
+
+ private ScoreScaler MIN_MAX_SCALER =
+ originScore -> (originScore - MIN_SCORE) / (MAX_SCORE - MIN_SCORE);
+
+ /**
+ * Given the calculated scores map by soft constraints, get the sum of scores
+ * @param originScoresMap The origin scores of soft constraints
+ * @return The sum of double type
+ */
+ double getSumOfScores(Map originScoresMap) {
Review comment:
Why we have inconsistent data type? If we think float is more efficient,
let's use float as return type. It is mixing up double/float in the code.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #422: Redefine the hard/soft constraints
dasahcc commented on a change in pull request #422: Redefine the hard/soft constraints URL: https://github.com/apache/helix/pull/422#discussion_r315475176 ## File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/SoftConstraintWeightModel.java ## @@ -0,0 +1,47 @@ +package org.apache.helix.controller.rebalancer.waged.constraints; Review comment: Have you set up the apache license header in the intellij? Once you set it up, it should be automatically pumped up. 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 #410: Fix the execution delay for the jobs (409)
narendly 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_r315474238
##
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestExecutionDelay.java
##
@@ -0,0 +1,132 @@
+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 com.google.common.collect.Lists;
+import java.util.List;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.TestHelper;
+import org.apache.helix.task.JobConfig;
+import org.apache.helix.task.JobContext;
+import org.apache.helix.task.JobQueue;
+import org.apache.helix.task.TaskConstants;
+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;
+
+/**
+ * Test To check if the Execution Delay is respected.
+ */
+public class TestExecutionDelay extends TaskTestBase {
+
+ @BeforeClass
+ public void beforeClass() throws Exception {
+super.beforeClass();
+ }
+
+ @Test
+ public void testExecutionDelay() throws Exception {
+// Execution Delay is set to be a large number. Hence, the workflow should
not be completed
+// soon.
+final long executionDelay = 1000L;
+String workflowName = TestHelper.getTestMethodName();
+Workflow.Builder builder = new Workflow.Builder(workflowName);
+
+// Workflow DAG Schematic:
+// JOB0
+// /\
+// / \
+// /\
+// JOB1 JOB2
+
+JobConfig.Builder jobBuilder =
JobConfig.Builder.fromMap(WorkflowGenerator.DEFAULT_JOB_CONFIG)
+.setMaxAttemptsPerTask(1).setWorkflow(workflowName)
+.setJobCommandConfigMap(ImmutableMap.of(MockTask.JOB_DELAY, "1000"));
+
+JobConfig.Builder jobBuilder2 =
JobConfig.Builder.fromMap(WorkflowGenerator.DEFAULT_JOB_CONFIG)
+.setMaxAttemptsPerTask(1).setWorkflow(workflowName)
+.setJobCommandConfigMap(ImmutableMap.of(MockTask.JOB_DELAY, "1000"));
+
+JobConfig.Builder jobBuilder3 =
JobConfig.Builder.fromMap(WorkflowGenerator.DEFAULT_JOB_CONFIG)
+.setMaxAttemptsPerTask(1).setWorkflow(workflowName)
+.setJobCommandConfigMap(ImmutableMap.of(MockTask.JOB_DELAY, "1000"));
+
+builder.addParentChildDependency("JOB0", "JOB1");
+builder.addParentChildDependency("JOB0", "JOB2");
+builder.addJob("JOB0", jobBuilder.setExecutionDelay(executionDelay));
+builder.addJob("JOB1", jobBuilder2);
+builder.addJob("JOB2", jobBuilder3);
+
+// Since the execution delay is set as a long time, it is expected to
reach time out.
+// If the workflow completed without any exception, it means that
ExecutionDelay has not been
+// respected.
+_driver.start(builder.build());
+
+// Verify workflow Context is created.
+// Wait until Workflow Context is created.
+boolean workflowCreated = TestHelper.verify(() -> {
+ WorkflowContext wCtx1 = _driver.getWorkflowContext(workflowName);
+ return wCtx1 != null;
+}, 30 * 1000);
+Assert.assertTrue(workflowCreated);
+
+// Verify StartTime is set for the Job0.
+// Wait until startTime is set.
+boolean startTimeSet = TestHelper.verify(() -> {
+ WorkflowContext wCtx1 = _driver.getWorkflowContext(workflowName);
+ long startTime = -1;
+ try {
+startTime =
wCtx1.getJobStartTime(TaskUtil.getNamespacedJobName(workflowName, "JOB0"));
+ } catch (Exception e) {
+// pass
+ }
+ return startTime != -1;
+}, 30 * 1000);
+Assert.assertTrue(startTimeSet);
+
+// Verify JobContext is not created. Creation of the JobContext means that
the job is ran or
+// in-progress. The absence of JobContext
[GitHub] [helix] i3wangyi commented on a change in pull request #422: Redefine the hard/soft constraints
i3wangyi commented on a change in pull request #422: Redefine the hard/soft
constraints
URL: https://github.com/apache/helix/pull/422#discussion_r315444535
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/SoftConstraint.java
##
@@ -27,23 +27,31 @@
* Evaluate a partition allocation proposal and return a score within the
normalized range.
* A higher score means the proposal is more preferred.
*/
-public interface SoftConstraint {
- float MIN_SCORE = -1000.0f;
- float MAX_SCORE = 1000.0f;
+public abstract class SoftConstraint {
+ private Type _type;
- /**
- * The scoring function returns a score between MINIMAL_SCORE and
MAXIMUM_SCORE, which is then weighted by the
- * individual normalized constraint weights.
- * Each individual constraint will define the meaning of MINIMAL_SCORE to
MAXIMUM_SCORE differently.
- */
- float assignmentScore(AssignableNode node, AssignableReplica rep,
ClusterContext clusterContext);
+ public SoftConstraint(Type type) {
+_type = type;
+ }
+
+ enum Type {
+LEAST_USED_NODE,
+LEAST_PARTITION_COUNT,
+LEAST_MOVEMENTS
+ }
/**
- * Set the importance factor of the soft constraint.
- * The more important it is, the more contribution it will make to the final
evaluation.
- * @param importance
+ * The scoring function returns a score between MINIMAL_SCORE and
MAXIMUM_SCORE, which is then
Review comment:
Just a comment wouldn't stop us from implementing any constraint at their
will. A stronger constraint would be to allow us to implement any ranged soft
constraints and the scaler scales the value to a reasonable range plus the
weight.
The MIN MAX scores are just copied from previous soft constraints'
interfaces. How about just use Float.MIN_VALUE and Float.MAX_VALUE?
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 #422: Redefine the hard/soft constraints
i3wangyi commented on a change in pull request #422: Redefine the hard/soft
constraints
URL: https://github.com/apache/helix/pull/422#discussion_r315442635
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/HardConstraint.java
##
@@ -37,14 +39,23 @@
NODES_CONTAIN_SAME_PARTITION,
}
+ public HardConstraint(Type failureReason) {
Review comment:
package-private makes more sense
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 #422: Redefine the hard/soft constraints
i3wangyi commented on a change in pull request #422: Redefine the hard/soft
constraints
URL: https://github.com/apache/helix/pull/422#discussion_r315439626
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/SoftConstraint.java
##
@@ -27,23 +27,31 @@
* Evaluate a partition allocation proposal and return a score within the
normalized range.
* A higher score means the proposal is more preferred.
*/
-public interface SoftConstraint {
- float MIN_SCORE = -1000.0f;
- float MAX_SCORE = 1000.0f;
+public abstract class SoftConstraint {
+ private Type _type;
- /**
- * The scoring function returns a score between MINIMAL_SCORE and
MAXIMUM_SCORE, which is then weighted by the
- * individual normalized constraint weights.
- * Each individual constraint will define the meaning of MINIMAL_SCORE to
MAXIMUM_SCORE differently.
- */
- float assignmentScore(AssignableNode node, AssignableReplica rep,
ClusterContext clusterContext);
+ public SoftConstraint(Type type) {
Review comment:
A default constructor in abstract class is a restriction that all the
subclasses have to extend and set the type field. This way subclass won't be
too casual without type.
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 #422: Redefine the hard/soft constraints
i3wangyi commented on a change in pull request #422: Redefine the hard/soft
constraints
URL: https://github.com/apache/helix/pull/422#discussion_r315439240
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/SoftConstraint.java
##
@@ -27,23 +27,31 @@
* Evaluate a partition allocation proposal and return a score within the
normalized range.
* A higher score means the proposal is more preferred.
*/
-public interface SoftConstraint {
- float MIN_SCORE = -1000.0f;
- float MAX_SCORE = 1000.0f;
+public abstract class SoftConstraint {
+ private Type _type;
- /**
- * The scoring function returns a score between MINIMAL_SCORE and
MAXIMUM_SCORE, which is then weighted by the
- * individual normalized constraint weights.
- * Each individual constraint will define the meaning of MINIMAL_SCORE to
MAXIMUM_SCORE differently.
- */
- float assignmentScore(AssignableNode node, AssignableReplica rep,
ClusterContext clusterContext);
+ public SoftConstraint(Type type) {
+_type = type;
+ }
+
+ enum Type {
Review comment:
Type and constraint need to be 1 to 1 mapping. How can we have one type
mapping to multiple constraints?
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 #422: Redefine the hard/soft constraints
i3wangyi commented on a change in pull request #422: Redefine the hard/soft
constraints
URL: https://github.com/apache/helix/pull/422#discussion_r315439144
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/SoftConstraintWeightModel.java
##
@@ -0,0 +1,47 @@
+package org.apache.helix.controller.rebalancer.waged.constraints;
+
+import java.util.Map;
+
+import com.google.common.collect.ImmutableMap;
+
+/**
+ * The class retrieves the offline model that defines the relative importance
of soft constraints.
+ */
+public class SoftConstraintWeightModel {
+ private float MIN_SCORE = -1000.0f;
+ private float MAX_SCORE = 1000.0f;
+ private static Map MODEL;
+
+ static {
+// TODO either define the weights in property files or zookeeper node or
static human input
+MODEL = ImmutableMap. builder()
+.put(SoftConstraint.Type.LEAST_MOVEMENTS, 1.0f)
+.put(SoftConstraint.Type.LEAST_PARTITION_COUNT, 1.0f)
+.put(SoftConstraint.Type.LEAST_USED_NODE, 1.0f).build();
+ }
+
+ interface ScoreScaler {
+/**
+ * Method to scale the origin score to a normalized range
+ * @param originScore The origin score of a range
+ * @return The normalized value between 0 - 1
+ */
+float scale(float originScore);
+ }
+
+ private ScoreScaler MIN_MAX_SCALER =
+ originScore -> (originScore - MIN_SCORE) / (MAX_SCORE - MIN_SCORE);
+
+ /**
+ * Given the calculated scores map by soft constraints, get the sum of scores
+ * @param originScoresMap The origin scores of soft constraints
+ * @return The sum of double type
+ */
+ public double sumOfScores(Map originScoresMap) {
+return originScoresMap.entrySet().stream().map(constraintToOriginScore -> {
+ float score = MIN_MAX_SCALER.scale(constraintToOriginScore.getValue());
+ float weight =
MODEL.getOrDefault(constraintToOriginScore.getKey().getType(), 0f);
Review comment:
The MODEL is defined statically and not set dynamically on the fly. I think
we should assume all types have a valid value or we're doing a bad job when
assigning the weight. Throw exception will fail all the rest operations I
don't think it's good idea
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-522772493 Please try to run "mvn test". 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 #422: Redefine the hard/soft constraints
jiajunwang commented on a change in pull request #422: Redefine the hard/soft
constraints
URL: https://github.com/apache/helix/pull/422#discussion_r315428362
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/HardConstraint.java
##
@@ -37,14 +39,23 @@
NODES_CONTAIN_SAME_PARTITION,
}
+ public HardConstraint(Type failureReason) {
+_type = failureReason;
+ }
+
+ public String getFailureReason() {
+// by default just return the type, extend the method if more details are
needed
+return _type.toString();
Review comment:
It might depends on the implementation logic.
We can have a default method with the basic message like this. So let's
comment that a child class shall override this method if necessary.
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 #422: Redefine the hard/soft constraints
jiajunwang commented on a change in pull request #422: Redefine the hard/soft
constraints
URL: https://github.com/apache/helix/pull/422#discussion_r315428431
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/HardConstraint.java
##
@@ -37,14 +39,23 @@
NODES_CONTAIN_SAME_PARTITION,
}
+ public HardConstraint(Type failureReason) {
Review comment:
protected?
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 #422: Redefine the hard/soft constraints
jiajunwang commented on a change in pull request #422: Redefine the hard/soft
constraints
URL: https://github.com/apache/helix/pull/422#discussion_r315424622
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/SoftConstraintWeightModel.java
##
@@ -0,0 +1,47 @@
+package org.apache.helix.controller.rebalancer.waged.constraints;
+
+import java.util.Map;
+
+import com.google.common.collect.ImmutableMap;
+
+/**
+ * The class retrieves the offline model that defines the relative importance
of soft constraints.
+ */
+public class SoftConstraintWeightModel {
+ private float MIN_SCORE = -1000.0f;
+ private float MAX_SCORE = 1000.0f;
+ private static Map MODEL;
+
+ static {
+// TODO either define the weights in property files or zookeeper node or
static human input
+MODEL = ImmutableMap. builder()
+.put(SoftConstraint.Type.LEAST_MOVEMENTS, 1.0f)
+.put(SoftConstraint.Type.LEAST_PARTITION_COUNT, 1.0f)
+.put(SoftConstraint.Type.LEAST_USED_NODE, 1.0f).build();
+ }
+
+ interface ScoreScaler {
+/**
+ * Method to scale the origin score to a normalized range
+ * @param originScore The origin score of a range
+ * @return The normalized value between 0 - 1
+ */
+float scale(float originScore);
+ }
+
+ private ScoreScaler MIN_MAX_SCALER =
+ originScore -> (originScore - MIN_SCORE) / (MAX_SCORE - MIN_SCORE);
+
+ /**
+ * Given the calculated scores map by soft constraints, get the sum of scores
+ * @param originScoresMap The origin scores of soft constraints
+ * @return The sum of double type
+ */
+ public double sumOfScores(Map originScoresMap) {
+return originScoresMap.entrySet().stream().map(constraintToOriginScore -> {
+ float score = MIN_MAX_SCALER.scale(constraintToOriginScore.getValue());
+ float weight =
MODEL.getOrDefault(constraintToOriginScore.getKey().getType(), 0f);
Review comment:
How to ensure the input map includes all the constraint evaluation result?
Shall we throw an exception if not?
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 #422: Redefine the hard/soft constraints
jiajunwang commented on a change in pull request #422: Redefine the hard/soft
constraints
URL: https://github.com/apache/helix/pull/422#discussion_r315429032
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/SoftConstraint.java
##
@@ -27,23 +27,31 @@
* Evaluate a partition allocation proposal and return a score within the
normalized range.
* A higher score means the proposal is more preferred.
*/
-public interface SoftConstraint {
- float MIN_SCORE = -1000.0f;
- float MAX_SCORE = 1000.0f;
+public abstract class SoftConstraint {
+ private Type _type;
Review comment:
final
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 #422: Redefine the hard/soft constraints
jiajunwang commented on a change in pull request #422: Redefine the hard/soft
constraints
URL: https://github.com/apache/helix/pull/422#discussion_r315426836
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/SoftConstraint.java
##
@@ -27,23 +27,31 @@
* Evaluate a partition allocation proposal and return a score within the
normalized range.
* A higher score means the proposal is more preferred.
*/
-public interface SoftConstraint {
- float MIN_SCORE = -1000.0f;
- float MAX_SCORE = 1000.0f;
+public abstract class SoftConstraint {
+ private Type _type;
- /**
- * The scoring function returns a score between MINIMAL_SCORE and
MAXIMUM_SCORE, which is then weighted by the
- * individual normalized constraint weights.
- * Each individual constraint will define the meaning of MINIMAL_SCORE to
MAXIMUM_SCORE differently.
- */
- float assignmentScore(AssignableNode node, AssignableReplica rep,
ClusterContext clusterContext);
+ public SoftConstraint(Type type) {
+_type = type;
+ }
+
+ enum Type {
+LEAST_USED_NODE,
+LEAST_PARTITION_COUNT,
+LEAST_MOVEMENTS
+ }
/**
- * Set the importance factor of the soft constraint.
- * The more important it is, the more contribution it will make to the final
evaluation.
- * @param importance
+ * The scoring function returns a score between MINIMAL_SCORE and
MAXIMUM_SCORE, which is then
Review comment:
Where is the definition of MINIMAL_SCORE and MAXIMUM_SCORE? What will be the
exception if this method returns any value out of the scope?
Please note in the comment.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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 #422: Redefine the hard/soft constraints
jiajunwang commented on a change in pull request #422: Redefine the hard/soft
constraints
URL: https://github.com/apache/helix/pull/422#discussion_r315427763
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/HardConstraint.java
##
@@ -27,8 +27,10 @@
* Evaluate a partition allocation proposal and return YES or NO based on the
cluster context.
* Any proposal fails one or more hard constraints will be rejected.
*/
-public interface HardConstraint {
- enum FailureReason {
+public abstract class HardConstraint {
+ private Type _type;
Review comment:
final
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 #422: Redefine the hard/soft constraints
jiajunwang commented on a change in pull request #422: Redefine the hard/soft
constraints
URL: https://github.com/apache/helix/pull/422#discussion_r315427077
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/HardConstraint.java
##
@@ -37,14 +39,23 @@
NODES_CONTAIN_SAME_PARTITION,
}
+ public HardConstraint(Type failureReason) {
+_type = failureReason;
+ }
+
+ public String getFailureReason() {
+// by default just return the type, extend the method if more details are
needed
+return _type.toString();
+ }
+
/**
- * @return True if the proposed assignment is valid.
+ * Check if the replica could be assigned to the node
+ * @return True if the proposed assignment is valid; False otherwise
*/
- boolean isAssignmentValid(AssignableNode node, AssignableReplica rep,
+ public abstract boolean isAssignmentValid(AssignableNode node,
AssignableReplica replica,
ClusterContext clusterContext);
- /**
- * @return Detail of the reason that the proposed assignment was rejected.
- */
- FailureReason getFailureReason();
+ public Type getType() {
Review comment:
protected shall be enough?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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 #422: Redefine the hard/soft constraints
jiajunwang commented on a change in pull request #422: Redefine the hard/soft
constraints
URL: https://github.com/apache/helix/pull/422#discussion_r315425900
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/SoftConstraint.java
##
@@ -27,23 +27,31 @@
* Evaluate a partition allocation proposal and return a score within the
normalized range.
* A higher score means the proposal is more preferred.
*/
-public interface SoftConstraint {
- float MIN_SCORE = -1000.0f;
- float MAX_SCORE = 1000.0f;
+public abstract class SoftConstraint {
+ private Type _type;
- /**
- * The scoring function returns a score between MINIMAL_SCORE and
MAXIMUM_SCORE, which is then weighted by the
- * individual normalized constraint weights.
- * Each individual constraint will define the meaning of MINIMAL_SCORE to
MAXIMUM_SCORE differently.
- */
- float assignmentScore(AssignableNode node, AssignableReplica rep,
ClusterContext clusterContext);
+ public SoftConstraint(Type type) {
Review comment:
Usually, we don't define a constructor for an abstract class. At least
should make it protected.
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 #422: Redefine the hard/soft constraints
jiajunwang commented on a change in pull request #422: Redefine the hard/soft
constraints
URL: https://github.com/apache/helix/pull/422#discussion_r315425615
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/SoftConstraint.java
##
@@ -27,23 +27,31 @@
* Evaluate a partition allocation proposal and return a score within the
normalized range.
* A higher score means the proposal is more preferred.
*/
-public interface SoftConstraint {
- float MIN_SCORE = -1000.0f;
- float MAX_SCORE = 1000.0f;
+public abstract class SoftConstraint {
+ private Type _type;
- /**
- * The scoring function returns a score between MINIMAL_SCORE and
MAXIMUM_SCORE, which is then weighted by the
- * individual normalized constraint weights.
- * Each individual constraint will define the meaning of MINIMAL_SCORE to
MAXIMUM_SCORE differently.
- */
- float assignmentScore(AssignableNode node, AssignableReplica rep,
ClusterContext clusterContext);
+ public SoftConstraint(Type type) {
+_type = type;
+ }
+
+ enum Type {
Review comment:
If we have 2 constraints with the same type defined, how to separately
weight them?
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 #422: Redefine the hard/soft constraints
jiajunwang commented on a change in pull request #422: Redefine the hard/soft
constraints
URL: https://github.com/apache/helix/pull/422#discussion_r315427001
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/SoftConstraint.java
##
@@ -27,23 +27,31 @@
* Evaluate a partition allocation proposal and return a score within the
normalized range.
* A higher score means the proposal is more preferred.
*/
-public interface SoftConstraint {
- float MIN_SCORE = -1000.0f;
- float MAX_SCORE = 1000.0f;
+public abstract class SoftConstraint {
+ private Type _type;
- /**
- * The scoring function returns a score between MINIMAL_SCORE and
MAXIMUM_SCORE, which is then weighted by the
- * individual normalized constraint weights.
- * Each individual constraint will define the meaning of MINIMAL_SCORE to
MAXIMUM_SCORE differently.
- */
- float assignmentScore(AssignableNode node, AssignableReplica rep,
ClusterContext clusterContext);
+ public SoftConstraint(Type type) {
+_type = type;
+ }
+
+ enum Type {
+LEAST_USED_NODE,
+LEAST_PARTITION_COUNT,
+LEAST_MOVEMENTS
+ }
/**
- * Set the importance factor of the soft constraint.
- * The more important it is, the more contribution it will make to the final
evaluation.
- * @param importance
+ * The scoring function returns a score between MINIMAL_SCORE and
MAXIMUM_SCORE, which is then
+ * weighted by the
+ * individual normalized constraint weights.
+ * Each individual constraint will define the meaning of MINIMAL_SCORE to
MAXIMUM_SCORE
+ * differently.
+ * @return float value representing the score
*/
- void setConstraintImportance(float importance);
+ public abstract float getAssignmentScore(AssignableNode node,
AssignableReplica rep,
+ ClusterContext clusterContext);
- float getConstraintImportance();
+ public Type getType() {
Review comment:
protected shall be enough?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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 #422: Redefine the hard/soft constraints
jiajunwang commented on a change in pull request #422: Redefine the hard/soft
constraints
URL: https://github.com/apache/helix/pull/422#discussion_r315428715
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/HardConstraint.java
##
@@ -27,8 +27,10 @@
* Evaluate a partition allocation proposal and return YES or NO based on the
cluster context.
* Any proposal fails one or more hard constraints will be rejected.
*/
-public interface HardConstraint {
- enum FailureReason {
+public abstract class HardConstraint {
+ private Type _type;
+
+ public enum Type {
Review comment:
Can we also make it protected? Not sure, just feel it would be cleaner.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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 #422: Redefine the hard/soft constraints
jiajunwang commented on a change in pull request #422: Redefine the hard/soft
constraints
URL: https://github.com/apache/helix/pull/422#discussion_r315427318
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/HardConstraint.java
##
@@ -37,14 +39,23 @@
NODES_CONTAIN_SAME_PARTITION,
}
+ public HardConstraint(Type failureReason) {
+_type = failureReason;
+ }
+
+ public String getFailureReason() {
+// by default just return the type, extend the method if more details are
needed
+return _type.toString();
+ }
+
/**
- * @return True if the proposed assignment is valid.
+ * Check if the replica could be assigned to the node
+ * @return True if the proposed assignment is valid; False otherwise
*/
- boolean isAssignmentValid(AssignableNode node, AssignableReplica rep,
+ public abstract boolean isAssignmentValid(AssignableNode node,
AssignableReplica replica,
Review comment:
Why we need it to be public here? Can we just put the algorithm
implementation class in the same package?
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_r315426751
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##
@@ -1,24 +1,144 @@
package org.apache.helix.rest.common;
-import org.apache.helix.HelixProperty;
-import org.apache.helix.PropertyKey;
-import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+/*
+ * 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.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.stream.Collectors;
+
+import org.apache.helix.HelixProperty;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+import org.apache.helix.model.RESTConfig;
+import org.apache.helix.rest.client.CustomRestClient;
+import org.apache.helix.rest.client.CustomRestClientFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* This is a wrapper for {@link ZKHelixDataAccessor} that caches the result of
the batch reads it
* performs.
* Note that the usage of this object is valid for one REST request.
*/
public class HelixDataAccessorWrapper extends ZKHelixDataAccessor {
+ private static final Logger LOG =
LoggerFactory.getLogger(HelixDataAccessorWrapper.class);
+ private static final ExecutorService POOL = Executors.newCachedThreadPool();
+
+ static final String PARTITION_HEALTH_KEY = "PARTITION_HEALTH";
+ static final String IS_HEALTHY_KEY = "IS_HEALTHY";
+ static final String EXPIRY_KEY = "EXPIRE";
+
private final Map _propertyCache = new
HashMap<>();
private final Map> _batchNameCache = new
HashMap<>();
+ protected CustomRestClient _restClient;
public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor) {
super(dataAccessor);
+_restClient = CustomRestClientFactory.get();
+ }
+
+ public Map>
getAllPartitionsHealthOnLiveInstance(RESTConfig restConfig,
+
Map customPayLoads) {
+// Only checks the instances are online with valid reports
+List liveInstances = getChildNames(keyBuilder().liveInstances());
+// Make a parallel batch call for getting all healthreports from ZK.
+List zkHealthReports = getProperty(liveInstances.stream()
+.map(instance -> keyBuilder().healthReport(instance,
PARTITION_HEALTH_KEY))
+.collect(Collectors.toList()), false);
+Map>> parallelTasks = new HashMap<>();
+for (int i = 0; i < liveInstances.size(); i++) {
+ String liveInstance = liveInstances.get(i);
+ Optional maybeHealthRecord = zkHealthReports != null &&
zkHealthReports.size() > i
Review comment:
The **getProperty** method alone is deprecated, instead I used
`getProperty(xxx, boolean throwException)`.
`List zkHealthReports` doesn't guarantee or doesn't
explicitly say that it will always be of equal size of input parameters. E.g,
I'd like to get 10 instances' healthReports, but the method only returns 5
objects. That's why I checked `zkHealthReports.size() > i` to avoid array index
of bounds issue.
To be extra safe,
It's possible the `zkHealthReports' is null, 'zkHealthReports' has less
items than live instances parameter and each object in `zkHealthReports' is
also possible null. All of these are covered in the ternary operator statement.
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
[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_r315423150
##
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;
+
+/**
+ * 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 {
Review comment:
You will need a ClusterModel to OptimalAssignment converting method here or
somewhere else. Note that if nothing has been changed since the previous
rebalance, the ClusterModel will contain all the assignment. And the
OptimalAssignment is expected to carry over these assignments. In this case, no
new assignment will be calculated.
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_r315421552
##
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;
+
+/**
+ * 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>>
_failedAssignments =
+ new HashMap<>();
+ private final ClusterContext _clusterContext;
+
+ public OptimalAssignment(ClusterContext clusterContext) {
+_clusterContext = clusterContext;
+ }
+
+ public ClusterContext getClusterContext() {
+return _clusterContext;
+ }
+
+ public Map getOptimalResourceAssignment() {
+// TODO: convert the optimal assignment to map
+return Collections.emptyMap();
+ }
+
+ public void trackAssignmentFailure(AssignableReplica replica,
+ Map> failedAssignments) {
+_failedAssignments.put(replica, failedAssignments);
+ }
+
+ public boolean hasAnyFailure() {
+return !_failedAssignments.isEmpty();
+ }
+
+ public String getFailures() {
+// TODO: format the error string
+return _failedAssignments.toString();
+ }
+
+ public void addAssignment(AssignableReplica replica, AssignableNode node) {
Review comment:
How you prevent the caller from sending a newly created node/replica object
as the input here? I mean an object that is not in the cluster context.
If anyone makes that mistake, the following logic might generate an
inconsistent result, right?
I strongly suggest you keep using the ClusterModel for now. Let's don't have
these update methods for now. As we talked, we can do this refactoring later as
a separate improvement.
If you want to finish this interface, you will need to add all the test
cases.
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 #421: Create definitions for soft/hard constraints and implementations
i3wangyi opened a new issue #421: Create definitions for soft/hard constraints and implementations URL: https://github.com/apache/helix/issues/421 For Waged Rebalancer 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_r315404402
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##
@@ -1,24 +1,144 @@
package org.apache.helix.rest.common;
-import org.apache.helix.HelixProperty;
-import org.apache.helix.PropertyKey;
-import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+/*
+ * 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.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.stream.Collectors;
+
+import org.apache.helix.HelixProperty;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+import org.apache.helix.model.RESTConfig;
+import org.apache.helix.rest.client.CustomRestClient;
+import org.apache.helix.rest.client.CustomRestClientFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* This is a wrapper for {@link ZKHelixDataAccessor} that caches the result of
the batch reads it
* performs.
* Note that the usage of this object is valid for one REST request.
*/
public class HelixDataAccessorWrapper extends ZKHelixDataAccessor {
+ private static final Logger LOG =
LoggerFactory.getLogger(HelixDataAccessorWrapper.class);
+ private static final ExecutorService POOL = Executors.newCachedThreadPool();
+
+ static final String PARTITION_HEALTH_KEY = "PARTITION_HEALTH";
+ static final String IS_HEALTHY_KEY = "IS_HEALTHY";
+ static final String EXPIRY_KEY = "EXPIRE";
+
private final Map _propertyCache = new
HashMap<>();
private final Map> _batchNameCache = new
HashMap<>();
+ protected CustomRestClient _restClient;
public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor) {
super(dataAccessor);
+_restClient = CustomRestClientFactory.get();
+ }
+
+ public Map>
getAllPartitionsHealthOnLiveInstance(RESTConfig restConfig,
+
Map customPayLoads) {
+// Only checks the instances are online with valid reports
+List liveInstances = getChildNames(keyBuilder().liveInstances());
+// Make a parallel batch call for getting all healthreports from ZK.
+List zkHealthReports = getProperty(liveInstances.stream()
+.map(instance -> keyBuilder().healthReport(instance,
PARTITION_HEALTH_KEY))
+.collect(Collectors.toList()), false);
+Map>> parallelTasks = new HashMap<>();
+for (int i = 0; i < liveInstances.size(); i++) {
+ String liveInstance = liveInstances.get(i);
+ Optional maybeHealthRecord = zkHealthReports != null &&
zkHealthReports.size() > i
Review comment:
How does zkHealthReports.size() > i work? I am confused here. Was that the
trick you did as .collect(Collectors.toList()), false)
If that's the case, can you add some comments here? It is hard to understand
the code in this way.
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 removed a comment on issue #417: Move partition health check method into dataAccessor layer
i3wangyi removed a comment on issue #417: Move partition health 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] narendly opened a new issue #420: Investigate race condition in job purging
narendly opened a new issue #420: Investigate race condition in job purging URL: https://github.com/apache/helix/issues/420 Investigate race condition in job purging 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_r315337089
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##
@@ -1,24 +1,158 @@
package org.apache.helix.rest.common;
-import org.apache.helix.HelixProperty;
-import org.apache.helix.PropertyKey;
-import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+/*
+ * 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.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.stream.Collectors;
+
+import org.apache.helix.HelixProperty;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.RESTConfig;
+import org.apache.helix.rest.client.CustomRestClient;
+import org.apache.helix.rest.client.CustomRestClientFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* This is a wrapper for {@link ZKHelixDataAccessor} that caches the result of
the batch reads it
* performs.
* Note that the usage of this object is valid for one REST request.
*/
public class HelixDataAccessorWrapper extends ZKHelixDataAccessor {
+ private static final Logger LOG =
LoggerFactory.getLogger(HelixDataAccessorWrapper.class);
+ private static final ExecutorService POOL = Executors.newCachedThreadPool();
+
+ private static final String PARTITION_HEALTH_KEY = "PARTITION_HEALTH";
+ private static final String IS_HEALTHY_KEY = "IS_HEALTHY";
+ private static final String EXPIRY_KEY = "EXPIRE";
+
+ private final CustomRestClient _restClient;
private final Map _propertyCache = new
HashMap<>();
private final Map> _batchNameCache = new
HashMap<>();
public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor) {
super(dataAccessor);
+_restClient = CustomRestClientFactory.get();
+ }
+
+ HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor, CustomRestClient
client) {
+super(dataAccessor);
+_restClient = client;
+ }
+
+ public List getExternalViews() {
+return getChildNames(keyBuilder().externalViews()).stream()
+.map(externalView -> (ExternalView)
getProperty(keyBuilder().externalView(externalView)))
+.collect(Collectors.toList());
+ }
+
+ public Map>
getPartitionHealthOfInstance(RESTConfig restConfig,
+ Map customPayLoads) {
+// Only checks the instances are online with valid reports
+List liveInstances = getChildNames(keyBuilder().liveInstances());
+// Make a parallel batch call for getting all healthreports from ZK.
+List zkHealthReports = getProperty(liveInstances.stream()
+.map(instance -> keyBuilder().healthReport(instance,
PARTITION_HEALTH_KEY))
+.collect(Collectors.toList()), false);
+Map>> parallelTasks = new HashMap<>();
+for (int i = 0; i < liveInstances.size(); i++) {
+ String liveInstance = liveInstances.get(i);
+ Optional maybeHealthRecord =
+
Optional.ofNullable(zkHealthReports.get(i)).map(HelixProperty::getRecord);
+ parallelTasks.put(liveInstance, POOL.submit(() -> {
+if (maybeHealthRecord.isPresent()) {
+ return getPartitionsHealth(liveInstance, maybeHealthRecord.get(),
restConfig,
+ customPayLoads);
+} else {
Review comment:
I am not saying totally remove the logic but just coding style. Since you
change it in java 8, I will review it again.
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
[GitHub] [helix] jiajunwang merged pull request #413: Add cluster level default instance config.
jiajunwang merged pull request #413: Add cluster level default instance config. URL: https://github.com/apache/helix/pull/413 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 #413: Add cluster level default instance config.
jiajunwang commented on issue #413: Add cluster level default instance config. URL: https://github.com/apache/helix/pull/413#issuecomment-522680174 > Before checking it in, do you think we should leave a comment on what would happen if neither instance capacity nor default value is set? Sure. Let me find a good place to comment on. 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_r315308557
##
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestExecutionDelay.java
##
@@ -0,0 +1,124 @@
+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 com.google.common.collect.Lists;
+import java.util.List;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.TestHelper;
+import org.apache.helix.task.JobConfig;
+import org.apache.helix.task.JobContext;
+import org.apache.helix.task.JobQueue;
+import org.apache.helix.task.TaskConstants;
+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;
+
+/**
+ * Test To check if the Execution Delay is respected.
+ */
+public class TestExecutionDelay extends TaskTestBase {
+
+ @BeforeClass
+ public void beforeClass() throws Exception {
+super.beforeClass();
+ }
+
+ @Test
+ public void testExecutionDelay() throws Exception {
+// Execution Delay is set to be a large number. Hence, the workflow should
not be completed
+// soon.
+final long executionDelay = 1000L;
+String workflowName = TestHelper.getTestMethodName();
+Workflow.Builder builder = new Workflow.Builder(workflowName);
+
+// Workflow DAG Schematic:
+// JOB0
+// /\
+// / \
+// /\
+// JOB1 JOB2
+
+JobConfig.Builder jobBuilder =
JobConfig.Builder.fromMap(WorkflowGenerator.DEFAULT_JOB_CONFIG)
+.setMaxAttemptsPerTask(1).setWorkflow(workflowName)
+.setJobCommandConfigMap(ImmutableMap.of(MockTask.JOB_DELAY, "1000"));
+
+JobConfig.Builder jobBuilder2 =
JobConfig.Builder.fromMap(WorkflowGenerator.DEFAULT_JOB_CONFIG)
+.setMaxAttemptsPerTask(1).setWorkflow(workflowName)
+.setJobCommandConfigMap(ImmutableMap.of(MockTask.JOB_DELAY, "1000"));
+
+JobConfig.Builder jobBuilder3 =
JobConfig.Builder.fromMap(WorkflowGenerator.DEFAULT_JOB_CONFIG)
+.setMaxAttemptsPerTask(1).setWorkflow(workflowName)
+.setJobCommandConfigMap(ImmutableMap.of(MockTask.JOB_DELAY, "1000"));
+
+builder.addParentChildDependency("JOB0", "JOB1");
+builder.addParentChildDependency("JOB0", "JOB2");
+builder.addJob("JOB0", jobBuilder.setExecutionDelay(executionDelay));
+builder.addJob("JOB1", jobBuilder2);
+builder.addJob("JOB2", jobBuilder3);
+
+// Since the execution delay is set as a long time, it is expected to
reach time out.
+// If the workflow completed without any exception, it means that
ExecutionDelay has not been
+// respected.
+_driver.start(builder.build());
+
+// Verify workflow Context is created.
+// Wait until Workflow Context is created.
+boolean resultWorkflow = TestHelper.verify(() -> {
+ WorkflowContext wCtx1 = _driver.getWorkflowContext(workflowName);
+ return (wCtx1 != null);
+}, 60 * 1000);
+Assert.assertTrue(resultWorkflow);
+
+// Verify StartTime is set for the Job.
+// Wait until startTime is set.
+boolean resultStartTime = TestHelper.verify(() -> {
+ WorkflowContext wCtx1 = _driver.getWorkflowContext(workflowName);
+ long startTime = -1;
+ try {
+startTime =
wCtx1.getJobStartTime(TaskUtil.getNamespacedJobName(workflowName, "JOB0"));
+ } catch (Exception e) {
+// pass
+ }
+ return (startTime != -1);
+}, 60 * 1000);
+Assert.assertTrue(resultStartTime);
+
+// Verify Job Context is not created. If Job Context is created, it means
that the execution
+// delay is not respected. if
[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_r315303268
##
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestExecutionDelay.java
##
@@ -0,0 +1,124 @@
+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 com.google.common.collect.Lists;
+import java.util.List;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.TestHelper;
+import org.apache.helix.task.JobConfig;
+import org.apache.helix.task.JobContext;
+import org.apache.helix.task.JobQueue;
+import org.apache.helix.task.TaskConstants;
+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;
+
+/**
+ * Test To check if the Execution Delay is respected.
+ */
+public class TestExecutionDelay extends TaskTestBase {
+
+ @BeforeClass
+ public void beforeClass() throws Exception {
+super.beforeClass();
+ }
+
+ @Test
+ public void testExecutionDelay() throws Exception {
+// Execution Delay is set to be a large number. Hence, the workflow should
not be completed
+// soon.
+final long executionDelay = 1000L;
+String workflowName = TestHelper.getTestMethodName();
+Workflow.Builder builder = new Workflow.Builder(workflowName);
+
+// Workflow DAG Schematic:
+// JOB0
+// /\
+// / \
+// /\
+// JOB1 JOB2
+
+JobConfig.Builder jobBuilder =
JobConfig.Builder.fromMap(WorkflowGenerator.DEFAULT_JOB_CONFIG)
+.setMaxAttemptsPerTask(1).setWorkflow(workflowName)
+.setJobCommandConfigMap(ImmutableMap.of(MockTask.JOB_DELAY, "1000"));
+
+JobConfig.Builder jobBuilder2 =
JobConfig.Builder.fromMap(WorkflowGenerator.DEFAULT_JOB_CONFIG)
+.setMaxAttemptsPerTask(1).setWorkflow(workflowName)
+.setJobCommandConfigMap(ImmutableMap.of(MockTask.JOB_DELAY, "1000"));
+
+JobConfig.Builder jobBuilder3 =
JobConfig.Builder.fromMap(WorkflowGenerator.DEFAULT_JOB_CONFIG)
+.setMaxAttemptsPerTask(1).setWorkflow(workflowName)
+.setJobCommandConfigMap(ImmutableMap.of(MockTask.JOB_DELAY, "1000"));
+
+builder.addParentChildDependency("JOB0", "JOB1");
+builder.addParentChildDependency("JOB0", "JOB2");
+builder.addJob("JOB0", jobBuilder.setExecutionDelay(executionDelay));
+builder.addJob("JOB1", jobBuilder2);
+builder.addJob("JOB2", jobBuilder3);
+
+// Since the execution delay is set as a long time, it is expected to
reach time out.
+// If the workflow completed without any exception, it means that
ExecutionDelay has not been
+// respected.
+_driver.start(builder.build());
+
+// Verify workflow Context is created.
+// Wait until Workflow Context is created.
+boolean resultWorkflow = TestHelper.verify(() -> {
+ WorkflowContext wCtx1 = _driver.getWorkflowContext(workflowName);
+ return (wCtx1 != null);
+}, 60 * 1000);
+Assert.assertTrue(resultWorkflow);
+
+// Verify StartTime is set for the Job.
+// Wait until startTime is set.
+boolean resultStartTime = TestHelper.verify(() -> {
+ WorkflowContext wCtx1 = _driver.getWorkflowContext(workflowName);
+ long startTime = -1;
+ try {
+startTime =
wCtx1.getJobStartTime(TaskUtil.getNamespacedJobName(workflowName, "JOB0"));
+ } catch (Exception e) {
+// pass
+ }
+ return (startTime != -1);
+}, 60 * 1000);
+Assert.assertTrue(resultStartTime);
+
+// Verify Job Context is not created. If Job Context is created, it means
that the execution
+// delay is not respected. if
[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_r315289200
##
File path:
helix-core/src/main/java/org/apache/helix/task/WorkflowDispatcher.java
##
@@ -602,4 +590,33 @@ private void removeContextsAndPreviousAssignment(String
workflow, Set jo
}
cache.removeContext(workflow);
}
+
+ private long computeStartTimeForJob(WorkflowContext workflowCtx, String job,
+ JobConfig jobConfig) {
+// Since the start time is calculated base on the time of completion of
parent jobs for this
+// job, the calculated start time should only be calculated once. Persist
the calculated time
+// in WorkflowContext znode.
+long calculatedStartTime = workflowCtx.getJobStartTime(job);
+if (calculatedStartTime < 0) {
+ // Calculate the start time if it is not already calculated
+ calculatedStartTime = System.currentTimeMillis();
+ // If the start time is not calculated before, do the math.
+ if (jobConfig.getExecutionDelay() >= 0) {
+calculatedStartTime += jobConfig.getExecutionDelay();
+ }
+ calculatedStartTime = Math.max(calculatedStartTime,
jobConfig.getExecutionStart());
+ workflowCtx.setJobStartTime(job, calculatedStartTime);
+}
+return calculatedStartTime;
+ }
+
+ private boolean isJobReadyToExecute(WorkflowContext workflowCtx, String job)
{
+long calculatedStartTime = workflowCtx.getJobStartTime(job);
+long timeNow = System.currentTimeMillis();
+if (timeNow < calculatedStartTime) {
Review comment:
There is no other checks it needed. I agree. I moved it inline to the caller
function.
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
