[GitHub] [dolphinscheduler] kezhenxu94 commented on a diff in pull request #10479: Fix TaskProcessorFactory#getTaskProcessor get common processor is not thread safe

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

kezhenxu94 commented on code in PR #10479:
URL: https://github.com/apache/dolphinscheduler/pull/10479#discussion_r898916672


##########
dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/entity/ProcessInstance.java:
##########
@@ -265,12 +265,13 @@ public ProcessInstance() {
      */
     public ProcessInstance(ProcessDefinition processDefinition) {
         this.processDefinition = processDefinition;
+        // todo: the name is not unique
         this.name = processDefinition.getName()
-            + "-"
-            +
-            processDefinition.getVersion()
-            + "-"
-            +
+                + "-"
+                +
+                processDefinition.getVersion()

Review Comment:
   nit: what about also simplify this with `String.join` or `Joiner` in guava, it should be improved in readability?



##########
dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/entity/ProcessInstance.java:
##########
@@ -265,12 +265,13 @@ public ProcessInstance() {
      */
     public ProcessInstance(ProcessDefinition processDefinition) {
         this.processDefinition = processDefinition;
+        // todo: the name is not unique

Review Comment:
   Can we do this in this PR?



##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/consumer/TaskPriorityQueueConsumer.java:
##########
@@ -190,16 +190,18 @@ protected boolean dispatchTask(TaskPriority taskPriority) {
                     return true;
                 }
             }
-
             result = dispatcher.dispatch(executionContext);
 
             if (result) {
+                logger.info("Master success dispatch task to worker, taskInstanceId: {}", taskPriority.getTaskId());
                 addDispatchEvent(context, executionContext);
+            } else {
+                logger.info("Master failed to dispatch task to worker, taskInstanceId: {}", taskPriority.getTaskId());
             }
         } catch (RuntimeException e) {
-            logger.error("dispatch error: ", e);
+            logger.error("Master dispatch task to worker error: ", e);
         } catch (ExecuteException e) {
-            logger.error("dispatch error: {}", e.getMessage());
+            logger.error("Master dispatch task to worker error: {}", e.getMessage());

Review Comment:
   nit: also change to this so we can get the exception stack?
   
   
   ```suggestion
               logger.error("Master dispatch task to worker error: {}", e);
   ```
   



##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/processor/queue/TaskEvent.java:
##########
@@ -27,10 +27,12 @@
 import java.util.Date;
 
 import io.netty.channel.Channel;
+import lombok.Data;
 
 /**
  * task event
  */
+@Data

Review Comment:
   💯 



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

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org