kaisun2000 commented on a change in pull request #1455:
URL: https://github.com/apache/helix/pull/1455#discussion_r502126203
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestDeleteJobFromJobQueue.java
##########
@@ -69,11 +69,18 @@ public void testForceDeleteJobFromJobQueue() throws
InterruptedException {
Assert
.assertNotNull(_driver.getJobContext(TaskUtil.getNamespacedJobName(jobQueueName,
"job2")));
- // The following force delete for the job should go through without
getting an exception
+ // force deletion can have Exception thrown as controller is writing to
propertystore path too.
+ // https://github.com/apache/helix/issues/1406
+
+ // Thus, we stop pipeline to make sure there is not such race condition.
+ _gSetupTool.getClusterManagementTool().enableCluster(CLUSTER_NAME, false);
+ Thread.sleep(3000); // note this sleep is critical as it would take time
for controller to stop
Review comment:
How do we verify the pipeline stops processing event? Note, you can't
just rely on pause signal node is in the Zookeeper.
This is similar to waitTillVerify(). It seems to me there is not good way to
wait for some signal.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestEnqueueJobs.java
##########
@@ -41,13 +47,39 @@
public void beforeClass() throws Exception {
setSingleTestEnvironment();
super.beforeClass();
+ //WorkflowConfig.disableJobPurge();
Review comment:
The should be removed.
Previously, we tried to set `protected static final long
DEFAULT_JOB_PURGE_INTERVAL = 30 * 60 * 1000; // default 30 minutes` to -1 in
WorkflowConfig.java, by using this function.
This has its drawback as people have concerns as it pollutes production code
as:
1/ need to make DEFAULT_JOB_PURGE_INTERVAL not final.
2/ The WorkflowConfig.disableJobPurge needs to be public. Thus, not testOnly.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TaskTestUtil.java
##########
@@ -61,12 +61,14 @@
import org.apache.helix.task.TaskUtil;
import org.apache.helix.task.WorkflowConfig;
import org.apache.helix.task.WorkflowContext;
+import org.slf4j.LoggerFactory;
import org.testng.Assert;
/**
* Static test utility methods.
*/
public class TaskTestUtil {
+ private static final org.slf4j.Logger logger =
LoggerFactory.getLogger(TaskTestUtil.class);
Review comment:
Changed to LOG
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TaskTestUtil.java
##########
@@ -231,9 +239,17 @@ public static Date getDateFromStartTime(String startTime)
public static JobQueue.Builder buildJobQueue(String jobQueueName, int
delayStart,
Review comment:
There are other places to use this function. If we don't have this
signature, that means probably a lot of usage places are also changed. That is
not essential change, make it hard for people to reason and may be later to
revert.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestDeleteJobFromJobQueue.java
##########
@@ -69,11 +69,18 @@ public void testForceDeleteJobFromJobQueue() throws
InterruptedException {
Assert
.assertNotNull(_driver.getJobContext(TaskUtil.getNamespacedJobName(jobQueueName,
"job2")));
- // The following force delete for the job should go through without
getting an exception
+ // force deletion can have Exception thrown as controller is writing to
propertystore path too.
+ // https://github.com/apache/helix/issues/1406
+
+ // Thus, we stop pipeline to make sure there is not such race condition.
+ _gSetupTool.getClusterManagementTool().enableCluster(CLUSTER_NAME, false);
+ Thread.sleep(3000); // note this sleep is critical as it would take time
for controller to stop
Review comment:
How do we verify the pipeline stops processing event? Note, you can't
just rely on pause signal node is in the Zookeeper.
This is similar to waitTillVerify(). It seems to me there is not good way to
wait for some signal using TestHelper.verify()
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestTaskRebalancerStopResume.java
##########
@@ -159,13 +190,16 @@ public void stopAndResumeNamedQueue() throws Exception {
verifyJobNotInQueue(queueName, namespacedJob2);
}
- @Test
+ @Test(enabled = true)
Review comment:
removed.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestTaskRebalancerStopResume.java
##########
@@ -32,6 +32,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
+import com.sun.corba.se.spi.orbutil.threadpool.Work;
Review comment:
removed.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestQuotaBasedScheduling.java
##########
@@ -491,7 +502,7 @@ public void testThreadLeak() throws InterruptedException {
* Tests quota-based scheduling for a job queue with different quota types.
* @throws InterruptedException
*/
- @Test(dependsOnMethods = "testSchedulingWithoutQuota")
+ @Test(dependsOnMethods = "testSchedulingWithoutQuota", enabled = true)
Review comment:
removed
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestEnqueueJobs.java
##########
@@ -65,12 +97,13 @@ public void testJobQueueAddingJobsOneByOne() throws
InterruptedException {
TaskState.COMPLETED);
}
- @Test
+ @Test()
Review comment:
removed.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestEnqueueJobs.java
##########
@@ -131,15 +165,16 @@ public void testJobSubmitGenericWorkflows() throws
InterruptedException {
_driver.pollForWorkflowState(workflowName, TaskState.COMPLETED);
}
- @Test
+ @Test()
Review comment:
restored.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestEnqueueJobs.java
##########
@@ -202,7 +237,7 @@ public void testQueueParallelJobs() throws
InterruptedException {
Assert.assertTrue(minFinishTime > maxStartTime);
}
- @Test
+ @Test()
Review comment:
restored.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestTaskRebalancerStopResume.java
##########
@@ -413,25 +447,38 @@ public void stopAndDeleteQueue() throws Exception {
// Create a queue
System.out.println("START " + queueName + " at " + new
Date(System.currentTimeMillis()));
WorkflowConfig wfCfg =
- new WorkflowConfig.Builder(queueName).setExpiry(2,
TimeUnit.MINUTES).build();
+ new WorkflowConfig.Builder(queueName).setExpiry(2, TimeUnit.MINUTES)
+ .setJobPurgeInterval(-1)
+ .build();
JobQueue qCfg = new
JobQueue.Builder(queueName).fromMap(wfCfg.getResourceConfigMap()).build();
_driver.createQueue(qCfg);
// Enqueue 2 jobs
+ List<String> jobNames = new ArrayList<>();
+ List<JobConfig.Builder> jobBuilders = new ArrayList<>();
+
Set<String> master = Sets.newHashSet("MASTER");
JobConfig.Builder job1 = new
JobConfig.Builder().setCommand(MockTask.TASK_COMMAND)
+ .setJobCommandConfigMap(Collections.singletonMap(MockTask.JOB_DELAY,
"2000"))
.setTargetResource(WorkflowGenerator.DEFAULT_TGT_DB).setTargetPartitionStates(master);
String job1Name = "masterJob";
LOG.info("Enqueuing job1: " + job1Name);
+ jobNames.add(job1Name);
+ jobBuilders.add(job1);
_driver.enqueueJob(queueName, job1Name, job1);
Set<String> slave = Sets.newHashSet("SLAVE");
JobConfig.Builder job2 = new
JobConfig.Builder().setCommand(MockTask.TASK_COMMAND)
+ .setJobCommandConfigMap(Collections.singletonMap(MockTask.JOB_DELAY,
"2000"))
.setTargetResource(WorkflowGenerator.DEFAULT_TGT_DB).setTargetPartitionStates(slave);
String job2Name = "slaveJob";
LOG.info("Enqueuing job2: " + job2Name);
+ jobNames.add(job2Name);
+ jobBuilders.add(job2);
_driver.enqueueJob(queueName, job2Name, job2);
+ //_driver.enqueueJobs(queueName, jobNames, jobBuilders);
Review comment:
removed.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestEnqueueJobs.java
##########
@@ -41,13 +47,39 @@
public void beforeClass() throws Exception {
setSingleTestEnvironment();
super.beforeClass();
+ //WorkflowConfig.disableJobPurge();
+ }
+
+ @AfterClass
+ @Override
+ public void afterClass() throws Exception {
+ //WorkflowConfig.enableJobPurge();
+ super.afterClass();
+ }
+
+ @BeforeMethod
+ public void beforeTest(Method testMethod, ITestContext testContext) {
Review comment:
Right. This should be removed.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestEnqueueJobs.java
##########
@@ -41,13 +47,39 @@
public void beforeClass() throws Exception {
setSingleTestEnvironment();
super.beforeClass();
+ //WorkflowConfig.disableJobPurge();
+ }
+
+ @AfterClass
+ @Override
+ public void afterClass() throws Exception {
+ //WorkflowConfig.enableJobPurge();
+ super.afterClass();
+ }
+
+ @BeforeMethod
+ public void beforeTest(Method testMethod, ITestContext testContext) {
+ long startTime = System.currentTimeMillis();
+ System.out.println("START " + testMethod.getName() + " at " + new
Date(startTime));
+ testContext.setAttribute("StartTime", System.currentTimeMillis());
+ }
+
+ @AfterMethod
+ public void endTest(Method testMethod, ITestContext testContext) {
Review comment:
Right. This should be removed.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestEnqueueJobs.java
##########
@@ -41,13 +47,39 @@
public void beforeClass() throws Exception {
setSingleTestEnvironment();
super.beforeClass();
+ //WorkflowConfig.disableJobPurge();
+ }
+
+ @AfterClass
+ @Override
+ public void afterClass() throws Exception {
Review comment:
These @beforeClass and @afterClass is due to the old way using
WorkflowConfig.disableJobPurge(). Should be removed.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestMaxNumberOfAttemptsMasterSwitch.java
##########
@@ -71,6 +71,9 @@ public void beforeClass() throws Exception {
@AfterClass
public void afterClass() throws Exception {
+ String testClassName = this.getShortClassName();
Review comment:
Good point. Moved to TaskSynchronizedTestBase @afterclass
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestQuotaBasedScheduling.java
##########
@@ -116,6 +117,11 @@ public void beforeClass() throws Exception {
_jobCommandMap = Maps.newHashMap();
}
+ @AfterClass
+ public void afterClass() throws Exception {
Review comment:
removed.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestStuckTaskQuota.java
##########
@@ -63,6 +63,9 @@ public void beforeClass() throws Exception {
@AfterClass
public void afterClass() throws Exception {
+ String testClassName = this.getShortClassName();
Review comment:
removed.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestTaskAssignmentCalculator.java
##########
@@ -102,6 +108,26 @@ public void beforeClass() throws Exception {
_jobCommandMap = Maps.newHashMap();
}
+ @AfterClass
+ public void afterClass() throws Exception {
+ super.afterClass();
+ }
+
+ @BeforeMethod
+ public void beforeTest(Method testMethod, ITestContext testContext) {
+ long startTime = System.currentTimeMillis();
+ System.out.println("START " + testMethod.getName() + " at " + new
Date(startTime));
+ testContext.setAttribute("StartTime", System.currentTimeMillis());
+ }
+
+ @AfterMethod
+ public void endTest(Method testMethod, ITestContext testContext) {
+ Long startTime = (Long) testContext.getAttribute("StartTime");
+ long endTime = System.currentTimeMillis();
+ System.out.println("END " + testMethod.getName() + " at " + new
Date(endTime) + ", took: "
+ + (endTime - startTime) + "ms.");
+ }
+
Review comment:
removed.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestTaskRebalancer.java
##########
@@ -43,9 +45,21 @@
import org.apache.helix.task.WorkflowConfig;
import org.apache.helix.task.WorkflowContext;
import org.testng.Assert;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
public class TestTaskRebalancer extends TaskTestBase {
+ @BeforeClass
+ public void beforeClass() throws Exception {
+ super.beforeClass();
+ }
+
+ @AfterClass
+ public void afterClass() throws Exception {
+ super.afterClass();
+ }
+
Review comment:
removed.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestTaskRebalancerParallel.java
##########
@@ -44,6 +45,11 @@ public void beforeClass() throws Exception {
super.beforeClass();
}
+ @AfterClass
Review comment:
removed.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestTaskRebalancerStopResume.java
##########
@@ -51,20 +52,36 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testng.Assert;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
public class TestTaskRebalancerStopResume extends TaskTestBase {
private static final Logger LOG =
LoggerFactory.getLogger(TestTaskRebalancerStopResume.class);
private static final String JOB_RESOURCE = "SomeJob";
+ @BeforeClass
Review comment:
removed.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestTaskSchedulingTwoCurrentStates.java
##########
@@ -93,6 +93,8 @@ public void beforeClass() throws Exception {
@AfterClass()
public void afterClass() throws Exception {
+ String testClassName = this.getShortClassName();
Review comment:
removed.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestTaskThrottling.java
##########
@@ -50,6 +52,10 @@ public void beforeClass() throws Exception {
super.beforeClass();
}
+ @AfterClass
+ public void afterClass() throws Exception {
Review comment:
removed.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestBatchAddJobs.java
##########
@@ -86,6 +86,9 @@ private String getPrefix(String job) {
@AfterClass
public void afterClass() {
+ String testClassName = this.getShortClassName();
+ System.out.println("AfterClass: " + testClassName + " of TestBachAddJobs
called.");
Review comment:
Changed to System.out.println("AfterClass: " + testClassName + "
called.");
Now changed to print in parent class, as suggested.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TaskTestUtil.java
##########
@@ -231,9 +239,17 @@ public static Date getDateFromStartTime(String startTime)
public static JobQueue.Builder buildJobQueue(String jobQueueName, int
delayStart,
int failureThreshold, int capacity) {
+ return buildJobQueue(jobQueueName, delayStart, failureThreshold, capacity,
false);
+ }
+
+ public static JobQueue.Builder buildJobQueue(String jobQueueName, int
delayStart,
+ int failureThreshold, int capacity, boolean disablePurge) {
Review comment:
Changed to do purge
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TaskTestUtil.java
##########
@@ -247,6 +263,11 @@ public static Date getDateFromStartTime(String startTime)
return new
JobQueue.Builder(jobQueueName).setWorkflowConfig(workflowCfgBuilder.build());
}
+ public static JobQueue.Builder buildJobQueue(String jobQueueName, int
delayStart,
Review comment:
changed to do purge
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestEnqueueJobs.java
##########
@@ -32,22 +34,23 @@
import org.apache.helix.task.Workflow;
import org.apache.helix.task.WorkflowConfig;
import org.testng.Assert;
+import org.testng.ITestContext;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.AfterMethod;
import org.testng.annotations.BeforeClass;
+import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
public class TestEnqueueJobs extends TaskTestBase {
- @BeforeClass
Review comment:
This should not be removed,
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
