[GitHub] [ignite-3] kgusakov commented on a change in pull request #175: Ignite-14890

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

kgusakov commented on a change in pull request #175:
URL: https://github.com/apache/ignite-3/pull/175#discussion_r654418172



##########
File path: modules/core/src/main/java/org/apache/ignite/lang/IgniteLogger.java
##########
@@ -55,15 +56,23 @@ protected IgniteLogger(@NotNull Class<?> cls) {
      * @param params Parameters.
      */
     public void info(String msg, Object... params) {
-        log.log(INFO, msg, params);
+        if (log.isLoggable(INFO)) {
+            IgniteBiTuple<String, Throwable> readyParams = format(msg, params);
+
+            log.log(INFO, readyParams.get1(), readyParams.get1());
+        }

Review comment:
       I made a small investigation around different logging interfaces  (jul, log4j, System.Logger) and it looks like only SLF4J using the doubtful approach with Throwable as the last param in varargs.
   
   So, I would prefer something like:
   `error(Supplier<String> msgSupplier, Throwable th)`
   and calling format manually on the user side. It must be a known API for users of modern JUL, log4j, System.Logger versions.
   
   But also we should provide shorthand versions for methods, which don't want to provide Throwable at all:
   `info(String format, Object... params)`
   and etc.
   
   WDYT @vldpyatkov 

##########
File path: modules/core/src/main/java/org/apache/ignite/lang/IgniteLogger.java
##########
@@ -55,15 +56,23 @@ protected IgniteLogger(@NotNull Class<?> cls) {
      * @param params Parameters.
      */
     public void info(String msg, Object... params) {
-        log.log(INFO, msg, params);
+        if (log.isLoggable(INFO)) {
+            IgniteBiTuple<String, Throwable> readyParams = format(msg, params);
+
+            log.log(INFO, readyParams.get1(), readyParams.get1());
+        }

Review comment:
       I made a small investigation around different logging interfaces  (jul, log4j, System.Logger) and it looks like only SLF4J using the doubtful approach with Throwable as the last param in varargs.
   
   So, I would prefer something like:
   `error(Supplier<String> msgSupplier, Throwable th)`
   and calling format manually on the user side. It must be a known API for users of modern JUL, log4j, System.Logger versions.
   
   But also we should provide shorthand versions for methods, which don't want to provide Throwable at all:
   `info(String format, Object... params)`
   and etc.
   
   WDYT @vldpyatkov?




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