[GitHub] [helix] narendly commented on a change in pull request #448: TASK: Fix forceDelete for jobs in JobQueue

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

narendly commented on a change in pull request #448: TASK: Fix forceDelete for jobs in JobQueue
URL: https://github.com/apache/helix/pull/448#discussion_r320569992
 
 

 ##########
 File path: helix-core/src/main/java/org/apache/helix/task/TaskDriver.java
 ##########
 @@ -262,32 +262,32 @@ public void deleteJob(final String queue, final String job, boolean forceDelete)
    * @param queue queue name
    * @param job job name: namespaced job name
    * @param forceDelete CAUTION: if set true, all job's related zk nodes will
-   *          be clean up from zookeeper even if its workflow information can not be found.
+   *          be removed from zookeeper even if its JobQueue information can not be found.
    */
   public void deleteNamespacedJob(final String queue, final String job, boolean forceDelete) {
-    WorkflowConfig workflowCfg = TaskUtil.getWorkflowConfig(_accessor, queue);
-
-    if (workflowCfg == null) {
-      if (forceDelete) {
-        // remove all job znodes if its original workflow config was already gone.
-        LOG.info("Forcefully removing job: " + job + " from queue: " + queue);
-        boolean success = TaskUtil.removeJob(_accessor, _propertyStore, job);
-        if (!success) {
-          LOG.info("Failed to delete job: " + job + " from queue: " + queue);
-          throw new HelixException("Failed to delete job: " + job + " from queue: " + queue);
-        }
-      } else {
-        throw new IllegalArgumentException("Queue " + queue + " does not yet exist!");
+    WorkflowConfig jobQueueConfig = TaskUtil.getWorkflowConfig(_accessor, queue);
+
+    if (forceDelete) {
+      // remove all job znodes if its original workflow config was already gone.
+      LOG.info("Forcefully removing job: {} from queue: {}", job, queue);
+      if (!TaskUtil.removeJob(_accessor, _propertyStore, job)) {
+        LOG.info("Failed to delete job: {} from queue: {}", job, queue);
+        throw new HelixException("Failed to delete job: " + job + " from queue: " + queue);
       }
       return;
     }
 
-    if (!workflowCfg.isJobQueue()) {
-      throw new IllegalArgumentException(queue + " is not a queue!");
+    if (jobQueueConfig == null) {
 
 Review comment:
   We want to do this check after we do force delete. For example, your queue might have been removed but for some reason the job ZNodes were not. In that case, you still want to be able to delete job ZNodes.

----------------------------------------------------------------
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


With regards,
Apache Git Services

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