[GitHub] [helix] dasahcc commented on a change in pull request #1565: Refine task framework log

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

dasahcc commented on a change in pull request #1565:
URL: https://github.com/apache/helix/pull/1565#discussion_r533655818



##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskDriver.java
##########
@@ -804,7 +803,9 @@ private void validateConfigsForTaskModifications(String workflowName, String job
     }
 
     if ((taskConfig.getCommand() == null) == (jobConfig.getCommand() == null)) {
-      throw new HelixException("Command must exist in either job or task, not both!");
+      throw new HelixException(String
+          .format("Command must exist in either job %s or task %s, not both!", jobName,
+              taskConfig.getId()));

Review comment:
       This may confusing. Originally it tells us the command should be either in job config or task config. But when you dd job name and task id can distract the focus. 
   
   You can change it as: "Command must exist in either jobconfig (jobname) or taskconfig (taskid)" something.

##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskDriver.java
##########
@@ -1065,16 +1066,16 @@ private void setSingleWorkflowTargetState(String workflow, final TargetState sta
 
     WorkflowConfig workflowConfig = TaskUtil.getWorkflowConfig(_accessor, workflow);
     if (workflowConfig == null) {
-      LOG.warn("WorkflowConfig for " + workflow + " not found!");
+      LOG.warn("WorkflowConfig for {} not found!", workflow);
       return;
     }
 
     WorkflowContext workflowContext = TaskUtil.getWorkflowContext(_propertyStore, workflow);
     if (state != TargetState.DELETE && workflowContext != null
         && workflowContext.getFinishTime() != WorkflowContext.UNFINISHED) {
       // Should not update target state for completed workflow
-      LOG.info("Workflow " + workflow + " is already completed, skip to update its target state "
-          + state);
+      LOG.info("Workflow {} is already completed, skip to update its target state {}",
+          workflow + state);

Review comment:
       This could cause runtime exception. Not workflow + state.

##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskDriver.java
##########
@@ -1173,7 +1176,7 @@ public TaskState pollForWorkflowState(String workflowName, long timeout,
 
     if (ctx == null || !allowedStates.contains(ctx.getWorkflowState())) {
       throw new HelixException(String.format(
-          "Workflow \"%s\" context is empty or not in states: \"%s\", current state: \"%s\"",
+          "Workflow %s context is empty or not in states: %s, current state: %s.",

Review comment:
       The quote gives us clear range in naming. If you dont like to see the escaper, then let's have "()" to bound the names.




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