[GitHub] [druid] paul-rogers commented on a diff in pull request #13790: Better logging for MSQ worker task

["paul-rogers (via GitHub)" <gi...@apache.org>]

paul-rogers commented on code in PR #13790:
URL: https://github.com/apache/druid/pull/13790#discussion_r1103876331


##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerImpl.java:
##########
@@ -331,8 +331,12 @@ public Optional<MSQErrorReport> runTask(final Closer closer) throws Exception
         final StageDefinition stageDefinition = kernel.getStageDefinition();
 
         if (kernel.getPhase() == WorkerStagePhase.NEW) {
-          log.debug("New work order: %s", context.jsonMapper().writeValueAsString(kernel.getWorkOrder()));
 
+          if (log.isDebugEnabled()) {
+            log.debug("Processing work order: %s", context.jsonMapper().writeValueAsString(kernel.getWorkOrder()));
+          } else {
+            log.info("Processing work order for stage[%d]", kernel.getStageDefinition().getStageNumber());

Review Comment:
   Would recommend first emitting the info-level general comment. If debug is enabled, emit the work order as well. Else when searching for this message, one has to know what log level was set to know which message to search for.



##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerImpl.java:
##########
@@ -520,6 +525,7 @@ public InputStream readChannel(
   @Override
   public void postWorkOrder(final WorkOrder workOrder)
   {
+    log.info("Got work order for stage[%d]", workOrder.getStageNumber());

Review Comment:
   Nit: better to use standard English spacing: a space between "stage" and the bracket: `stage [%d]`.
   
   The brackets are really only needed for items that can contain spaces, so we know what is the message and what is the value. But, this is a number. so `state %d`.



##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerImpl.java:
##########
@@ -603,6 +611,11 @@ public ClusterByStatisticsSnapshot fetchStatisticsSnapshot(StageId stageId)
   @Override
   public ClusterByStatisticsSnapshot fetchStatisticsSnapshotForTimeChunk(StageId stageId, long timeChunk)
   {
+    log.debug(
+        "Fetching statistics for stage:[%d] with timechunk:[%d]",

Review Comment:
   Edit as above `stage %d, time chunk %d`



##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerImpl.java:
##########
@@ -444,6 +448,7 @@ public Optional<MSQErrorReport> runTask(final Closer closer) throws Exception
   @Override
   public void stopGracefully()
   {
+    log.info("Stopping gracefully");

Review Comment:
   Does the logger emit the task ID? If not, it might help to say what is stopping gracefully.



##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerImpl.java:
##########
@@ -582,12 +588,14 @@ public void postCleanupStage(final StageId stageId)
   @Override
   public void postFinish()
   {
+    log.info("Finish received");
     kernelManipulationQueue.add(KernelHolder::setDone);
   }
 
   @Override
   public ClusterByStatisticsSnapshot fetchStatisticsSnapshot(StageId stageId)
   {
+    log.info("Fetching statistics for stage:[%d]", stageId.getStageNumber());

Review Comment:
   As above. Inconsistent use of colon. No colon is needed here: `stage %d`.
   
   Does the logger automagically fill in the task ID? If not, would be good to include that so we can find events for a specific task.



##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/kernel/worker/WorkerStageKernel.java:
##########
@@ -210,6 +212,12 @@ private void assertPreshuffleStatisticsNeeded()
   private void transitionTo(final WorkerStagePhase newPhase)
   {
     if (newPhase.canTransitionFrom(phase)) {
+      log.info(

Review Comment:
   Same comments as above.



##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerImpl.java:
##########
@@ -582,12 +588,14 @@ public void postCleanupStage(final StageId stageId)
   @Override
   public void postFinish()
   {
+    log.info("Finish received");

Review Comment:
   Finish received for what task?



-- 
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@druid.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org