[GitHub] [druid] FrankChen021 commented on pull request #12097: perf: eliminate double string concat in remote-task-runner shutdown logging

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

FrankChen021 commented on pull request #12097:
URL: https://github.com/apache/druid/pull/12097#issuecomment-1000600708


   Since the `reason` is passed to `shutdown` only for logging, why do we do what you have changed in the interface so that all subclasses gain this improvement?
   
   Something ike this:
   
   ```
   Logger getLogger();
   
   default void shutdown(String taskid, String reasonFormat, Object... args) 
    { 
      getLogger().info("Shutdown [%s] because: [" + reasonFormat + "]" + reasonFormat, taskid, args);
      shutdown(taskid); 
    } 
   ```
   
   
   Another thing that confuses me is that the flamechart shows that there's some bottlenecks at the logging, not the `StringUtils.format`, so to gain the performance improvement, the right thing here to do is to change the logging level to WARN or ERROR right? What you solved here is just to eliminate some string construction under WARN or ERROR logging level, right? 
   
   In another word, at INFO logging level, the gain seems not significant.


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