jiajunwang commented on a change in pull request #1455:
URL: https://github.com/apache/helix/pull/1455#discussion_r502106398
##########
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:
nit, should be "LOG" based on our naming convention.
##########
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:
This is a test method, we don't need to ensure backward compatibility.
How about just change the method?
##########
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:
What is this?
##########
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:
Please confirm if this has already been done by the parent class
ZkTestBase.
I think this is duplicate logic.
##########
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:
Same here
##########
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:
Isn't enabled default to be true?
##########
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:
Same here.
##########
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:
Humm, testClassName should be "TestBachAddJobs", right? So it will be
"AfterClass: TestBachAddJobs of TestBachAddJobs called."?
Moreover, why we add this logic to every child test class instead of print
in the parent class?
##########
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:
I think you also emphasized that we shall avoid using Thread.sleep() in
the tests. 3-second sleep is not guaranteed to work.
Can we use verifier with a timeout to wait until the pipeline stopped?
##########
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:
Same here
##########
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:
It's OK, but why?
##########
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:
I did a quick test, I think you can just remove it and the
super.afterClass() will still be called, since you don't have any method with
the same name to override it in that case.
##########
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:
Samething here, ZKTestBase has this method with same logic already.
##########
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:
Same here
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestQuotaBasedScheduling.java
##########
@@ -135,6 +141,7 @@ public void testSchedulingWithoutQuota() throws
InterruptedException {
Workflow.Builder workflowBuilder = new Workflow.Builder(workflowName);
WorkflowConfig.Builder configBuilder = new
WorkflowConfig.Builder(workflowName);
configBuilder.setAllowOverlapJobAssignment(true);
+ configBuilder.setJobPurgeInterval(-1);
Review comment:
Are you suggest we remove these changes one by one if later we fix the
purge issue?
@NealSun96 , what is the ETA of your change, please? I think removing these
changes will be troublesome. Unless we separate them into a single change and
revert it completely later.
##########
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:
Instead of doing this, I suggest refactoring the afterClass() method in
TaskSynchronizedTestBase or ZkTestBase. In this case, you don't have duplicate
code. And all the child classes have it automatically.
##########
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:
nit, but it would be more intuitive to be "boolean doPurge"
##########
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:
I think it is not necessary. The super afterClass will still be called
if there is no class with the same name override it in the child class. Please
have a try.
##########
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:
Humm, interesting package. What is the usage, please?
##########
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:
Same here, try to update parent class please.
##########
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:
Same here.
##########
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:
default is true.
/**
* Whether methods on this class/method are enabled.
*/
public boolean enabled() default true;
##########
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:
Same here, I think if the classes are defined carefully, then the parent
class logic can cover these.
##########
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:
As we can see, there are many changes like this. So let's do it in the
parent class. Shall we?
##########
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:
Same here. Are you sure we need them?
##########
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"))
Review comment:
I'm not sure if we really need it. But looks harmless.
@alirezazamani @NealSun96 , could you please help to take another look?
##########
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:
???
##########
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:
Same 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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
