[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] 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_r315043967
##
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 not
[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_r315043520
##
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(() -> {
Review comment:
Minor:
Boolean naming convention
https://stackoverflow.com/questions/40339847/variable-and-method-naming-conventions-for-boolean-verbs-in-java
workflowCreated might be a better option here?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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_r315043805
##
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);
Review comment:
parentheses not needed.
This is an automated message from the Apache Git Service.
To respond to the message,
[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_r315044176
##
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 not
[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_r315042991
##
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:
This method could be simplified to:
`return System.currentTimeMillis() >= workflowCtx.getJobStartTime(job);`
Also, are we sure that there's no other checks required in this function? If
it's just going to be a one-liner (in other words, there are no other checks to
perform than checking the calculatedStartTime against current time to determine
if a job is ready to be processed/updated), and if the usage is only in this
place, perhaps it may not be worth it to make it a separate function. If that's
the case, let's just keep what we had inline.
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_r315044366
##
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 not
[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_r314872550
##
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 (isJobReadyToExecute(workflowCtx, inflightJob)) {
Review comment:
@dasahcc I discussed with @alirezazamani offline. I feel that we just
neglected all the checking required before calling `processJob` on inflight
jobs. "Inflight" in this sense doesn't mean that a job is necessarily running;
rather, it means that the job has been passed over onto the Controller and now
out of RuntimeJobDAG's responsibility. Otherwise _we end up doing a lot of
"passing jobs back and forth" for every failure/delay situation_ that will
complicate the code/logic even further.
So in short, it was a simple bug that we didn't check before doing
processJob here.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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_r314763636
##
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestExecutionDelay.java
##
@@ -0,0 +1,82 @@
+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;
+
+/**
+ * Test To check if the Execution Delay is respected.
+ * If Workflow is completed regardless of execution delay, it means this task
should be failed.
+ * If execution delay is respected, the task will be passed.
+ */
+public class TestExecutionDelay extends TaskTestBase {
+
+ @BeforeClass
+ public void beforeClass() throws Exception {
+super.beforeClass();
+ }
+
+ @Test(expectedExceptions = HelixException.class)
+ public void testExecutionDelay() throws InterruptedException {
+final long timeOutTime = 15000L;
Review comment:
One might wonder, why 15 sec?
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_r314758928
##
File path:
helix-core/src/main/java/org/apache/helix/task/WorkflowDispatcher.java
##
@@ -602,4 +590,28 @@ private void removeContextsAndPreviousAssignment(String
workflow, Set jo
}
cache.removeContext(workflow);
}
+
+ private long calculateStartTime(WorkflowContext workflowCtx, String job,
JobConfig jobConfig) {
+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)
{
+if (System.currentTimeMillis() < workflowCtx.getJobStartTime(job)) {
Review comment:
Simplify the boolean condition?
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_r314766079
##
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestExecutionDelay.java
##
@@ -0,0 +1,82 @@
+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;
+
+/**
+ * Test To check if the Execution Delay is respected.
+ * If Workflow is completed regardless of execution delay, it means this task
should be failed.
+ * If execution delay is respected, the task will be passed.
+ */
+public class TestExecutionDelay extends TaskTestBase {
+
+ @BeforeClass
+ public void beforeClass() throws Exception {
+super.beforeClass();
+ }
+
+ @Test(expectedExceptions = HelixException.class)
+ public void testExecutionDelay() throws InterruptedException {
+final long timeOutTime = 15000L;
+final long executionDelay = 1L;
+String workflowName = TestHelper.getTestMethodName();
+Workflow.Builder builder = new Workflow.Builder(workflowName);
+
+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());
+_driver.pollForWorkflowState(workflowName, timeOutTime,
TaskState.COMPLETED);
Review comment:
I think there's a better way to test this. Instead of relying on
pollForWorkflowState timing out, could you check the status of the workflow
**and** the individual jobs? What if the controller is very slow and takes 15
seconds to schedule jobs?
One way of not relying on timing is
1. Start workflow
2. Wait until it is in progress and job 0 has been looked at (controller
tried to schedule)
3. Read the context and check start time is set with the right amount
4. Check that job 0 and its children jobs have not run by checking their
states
This way, your test is robust against timing issues, correct? No matter how
long controller takes to start and schedule that first job instead of relying
on 15 sec, an arbitrarily long enough number? :)
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
[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_r314762972
##
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestExecutionDelay.java
##
@@ -0,0 +1,82 @@
+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;
+
+/**
+ * Test To check if the Execution Delay is respected.
+ * If Workflow is completed regardless of execution delay, it means this task
should be failed.
+ * If execution delay is respected, the task will be passed.
+ */
+public class TestExecutionDelay extends TaskTestBase {
+
+ @BeforeClass
+ public void beforeClass() throws Exception {
+super.beforeClass();
+ }
+
+ @Test(expectedExceptions = HelixException.class)
+ public void testExecutionDelay() throws InterruptedException {
+final long timeOutTime = 15000L;
+final long executionDelay = 1L;
+String workflowName = TestHelper.getTestMethodName();
+Workflow.Builder builder = new Workflow.Builder(workflowName);
+
+JobConfig.Builder jobBuilder =
JobConfig.Builder.fromMap(WorkflowGenerator.DEFAULT_JOB_CONFIG)
Review comment:
Add a block comment on what DAG you're creating. Something like
```
// 1
// /\
// 2 3
```
You get the idea? Helps others understand what you're doing.
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_r314761291
##
File path:
helix-core/src/main/java/org/apache/helix/task/WorkflowDispatcher.java
##
@@ -602,4 +590,28 @@ private void removeContextsAndPreviousAssignment(String
workflow, Set jo
}
cache.removeContext(workflow);
}
+
+ private long calculateStartTime(WorkflowContext workflowCtx, String job,
JobConfig jobConfig) {
Review comment:
I think computeStartTimeForJob or something like that is more intuitive and
specific. What do you think?
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_r314177297
##
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:
For the execution delay itself, adding this check may do the trick. However,
two more things to consider:
1. Would this be the right place to place the check? Say, should we include
this check inside processJob()? Or should we create a separate method that
checks whether a job is eligible to be processed?
2. Could there be more checks we need to perform other than execution delay?
For example, retry delay, etc. It would be great if we could make this a
comprehensive fix :)
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_r314176167
##
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()
+ throws InterruptedException {
+String workflowName = TestHelper.getTestMethodName();
+Workflow.Builder builder = new Workflow.Builder(workflowName);
+
+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(1L));
+builder.addJob("JOB1", jobBuilder2);
+builder.addJob("JOB2", jobBuilder3);
+
+_driver.start(builder.build());
+
+_driver.pollForWorkflowState(workflowName, 15000, TaskState.COMPLETED);
+
+ }
+
Review comment:
Could we remove this empty line? :)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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_r314176056
##
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:
Style - annotation should take up a line on its own
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_r314176558
##
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:
Where is this HelixAdmin used?
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_r314176155
##
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()
+ throws InterruptedException {
+String workflowName = TestHelper.getTestMethodName();
+Workflow.Builder builder = new Workflow.Builder(workflowName);
+
+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(1L));
+builder.addJob("JOB1", jobBuilder2);
+builder.addJob("JOB2", jobBuilder3);
+
+_driver.start(builder.build());
+
+_driver.pollForWorkflowState(workflowName, 15000, TaskState.COMPLETED);
+
Review comment:
Could we remove this empty line? :)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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_r314176632
##
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()
+ throws InterruptedException {
+String workflowName = TestHelper.getTestMethodName();
+Workflow.Builder builder = new Workflow.Builder(workflowName);
+
+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(1L));
Review comment:
Magic number -> make it a constant?
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_r314176396
##
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()
+ throws InterruptedException {
+String workflowName = TestHelper.getTestMethodName();
+Workflow.Builder builder = new Workflow.Builder(workflowName);
+
+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(1L));
+builder.addJob("JOB1", jobBuilder2);
+builder.addJob("JOB2", jobBuilder3);
+
+_driver.start(builder.build());
+
+_driver.pollForWorkflowState(workflowName, 15000, TaskState.COMPLETED);
Review comment:
For longs, please append "L" at the end.
`15000 -> 15000L`
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_r314177297
##
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:
For the execution delay itself, adding this check may do the trick. However,
two more things to consider:
1. Would this be the right place to place the check? Say, should we include
this check inside processJob()? Or should we create a separate method that
checks whether a job is eligible to be processed?
2. Could there be more checks we need to perform other than execution delay?
It would be great if we could make this a comprehensive fix :)
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_r314176671
##
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()
+ throws InterruptedException {
+String workflowName = TestHelper.getTestMethodName();
+Workflow.Builder builder = new Workflow.Builder(workflowName);
+
+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(1L));
+builder.addJob("JOB1", jobBuilder2);
+builder.addJob("JOB2", jobBuilder3);
+
+_driver.start(builder.build());
+
+_driver.pollForWorkflowState(workflowName, 15000, TaskState.COMPLETED);
Review comment:
Magic number - make it a constant?
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_r314176020
##
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:
Minor: usually superclass's methods are called first.
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_r314175964
##
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 {
Review comment:
Did you format the diff with helix-style? @BeforeClass should take up a line
to itself.
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_r314175903
##
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:
Let's use an underscore for private variables.
`admin -> _admin`
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
