[GitHub] [helix] jiajunwang commented on a change in pull request #1548: Fix targeted job quota calculation for given up tasks

[GitBox <gi...@apache.org>]

jiajunwang commented on a change in pull request #1548:
URL: https://github.com/apache/helix/pull/1548#discussion_r528987288



##########
File path: helix-core/src/test/java/org/apache/helix/integration/task/TestTaskErrorMaxRetriesQuotaRelease.java
##########
@@ -0,0 +1,63 @@
+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 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.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import com.google.common.collect.ImmutableMap;
+
+public class TestTaskErrorMaxRetriesQuotaRelease extends TaskTestBase {
+
+  @BeforeClass
+  public void beforeClass() throws Exception {
+    _numNodes = 1;
+    _numPartitions = 100;
+    super.beforeClass();
+  }
+
+  @AfterClass
+  public void afterClass() throws Exception {
+    super.afterClass();

Review comment:
       This is not necessary. If you don't have any additional logic, then you can remove the method in the child class. And the parent class afterClass() will be triggered.

##########
File path: helix-core/src/test/java/org/apache/helix/integration/task/TestTaskErrorMaxRetriesQuotaRelease.java
##########
@@ -0,0 +1,63 @@
+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 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.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import com.google.common.collect.ImmutableMap;
+
+public class TestTaskErrorMaxRetriesQuotaRelease extends TaskTestBase {

Review comment:
       Is it possible to add the method to an existing test class such as "TestQuotaBasedScheduling"? Note every new test class will require a new cluster to be recreated and removed. In most cases, it is not necessary.

##########
File path: helix-core/src/main/java/org/apache/helix/task/AbstractTaskDispatcher.java
##########
@@ -838,7 +839,7 @@ private static void addCompletedTasks(Set<Integer> set, JobContext ctx, Iterable
    */
   private boolean isTaskNotInTerminalState(TaskPartitionState state) {

Review comment:
       This change also impacts filterTasks(). The logic looks to be valid even after this change.
   The concerning part is that there are some duplicate checks here and there. For example,
   
         // Allow tasks eligible for scheduling
         if (state == null || state == TaskPartitionState.STOPPED
             || state == TaskPartitionState.TIMED_OUT || state == TaskPartitionState.TASK_ERROR
             || state == TaskPartitionState.DROPPED) {
           filteredTasks.add(partitionNumber);
         }
         // Allow tasks whose assigned instances are no longer live for rescheduling
         if (isTaskNotInTerminalState(state)) {
           ...
         }
   
   This section checks for different state combinations and then react differently. First of all, it is hard for the reviewers to follow the logic. Secondly, it is very easy for the coder to introduce bugs since the combinations are separately done in different private methods.
   Is it possible that we conclude all the states check within 3 or 4 methods? Or we need to clean up the state. The current design is not reviewable. 




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org