[GitHub] [daffodil] mbeckerle commented on a change in pull request #605: Replace logging infrastructure with Log4j

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

mbeckerle commented on a change in pull request #605:
URL: https://github.com/apache/daffodil/pull/605#discussion_r666435862



##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/DFDLFormatAnnotation.scala
##########
@@ -146,7 +146,7 @@ abstract class DFDLFormatAnnotation(nodeArg: Node, annotatedSCArg: AnnotatedSche
           val defFmt = schemaSet.getDefineFormat(adjustedQN).getOrElse {
             annotatedSC.schemaDefinitionError("defineFormat with name '%s', was not found.", adjustedQN.toString)
           }
-          log(LogLevel.Debug, "found defineFormat named: %s", adjustedQN)
+          Logger.log.debug(s"found defineFormat named: ${adjustedQN}")

Review comment:
       I would just delete this one. 

##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Logger.scala
##########
@@ -17,213 +17,30 @@
 
 package org.apache.daffodil.util
 
-import java.text.SimpleDateFormat
-import java.util.Date
-import java.io.File
-import java.io.PrintStream
-import java.io.FileOutputStream
-import org.apache.daffodil.exceptions.Assert
-import org.apache.daffodil.util._
-import org.apache.daffodil.util.Maybe._
-import org.apache.daffodil.exceptions.UnsuppressableException
+import org.apache.logging.log4j.scala.Logging
 
 /**
- * Simple logging system evolved from code found on Stack Overflow, on the web.
- * http://stackoverflow.com/questions/2018528/logging-in-scala
- * Mostly based on the contribution of Don Mackenzie.
+ * The log4j.scala.Logging trait adds a log4j.scala.Logger member val called
+ * 'logger' to whatever class mixes it in. Classes that mixin this trait can
+ * then just call logger.warn/info/debug/etc to log a message, which are macros
+ * to minimize overhead.
  *
- * Extensively modified to use Macros for performance now.
+ * However, the Logger class is not serializable, and so all classes that might
+ * need to be serialized, (e.g. runtime objects), cannot take this approach.
+ *
+ * Instead, we have a single Logger object that mixes in this trait. This
+ * object is never serialized and so can be used anywhere, regardless of
+ * serializablility. For simplicity, consistency, and to avoid potential
+ * serialization issues, all logging should be done via the logger in this
+ * object, rather than mixing in the Logging trait, for example:
+ *
+ *   Logger.log.info("Message to log")
+ *
+ * The downside to this is that it breaks the ability to use Log4j's feature to
+ * configure different log levels for different namespaces or classes or have
+ * useful line numbers, because all logging comes from and is associated with
+ * this single Logger in the 'org.apache.daffodil.util.Logger' class.

Review comment:
       We could have multiple logger objects if we wanted to, so as to get some of these capabilities back. Not sure we need it, but it seems possible. 

##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/DFDLFormatAnnotation.scala
##########
@@ -178,7 +178,7 @@ abstract class DFDLFormatAnnotation(nodeArg: Node, annotatedSCArg: AnnotatedSche
    */

Review comment:
       Suggest just delete this entirely.

##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/compiler/Compiler.scala
##########
@@ -352,15 +351,15 @@ class Compiler private (var validateDFDLSchemas: Boolean,
     val diags = pf.getDiagnostics // might be warnings even if not isError
     if (err) {
       Assert.invariant(diags.nonEmpty)
-      log(LogLevel.Compile, "Compilation (ProcessorFactory) produced %d errors/warnings.", diags.length)
+      Logger.log.debug(s"Compilation (ProcessorFactory) produced ${diags.length} errors/warnings.")

Review comment:
       This style fundamentally bothers me. It is incompatible with Internationalization of the logging. 
   However, unlike error messages like our SDE/SDW/PE/UE/VE errors, logging quite possibly should be English only, so this is ok, 
   
   I just like the idea of having only one idiom for messaging - whether it is logging, issuing a SDE, or actually printing something out. 
   
   I always prefer this style:
   ```
   loggingOrOtherMesageCreator("Message in English text with %s placeholders", arg0, arg1, arg2...argK)
   ```
   as the style because it enables internationalization by using the Message string as a lookup key into a database by desired locale, and the object retrieved can scrutinize arguments to choose correct text. 




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

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