> On July 28, 2016, 8:42 p.m., Xinyu Liu wrote: > > samza-core/src/main/scala/org/apache/samza/util/Logging.scala, line 32 > > <https://reviews.apache.org/r/50583/diff/1/?file=1456824#file1456824line32> > > > > Nit: this method does info log which is not very straghtforward from > > the name, and also the name "Log" is not consistent with the other methods. > > Do you think it's better to have: > > > > Logging.info((message, startupLog:boolean=false) => Any) > > > > That way we can use info log for both startup and non startup logging, > > and potentially adding it for debug and error in the future too.
I had similar feedback in the original review: https://reviews.apache.org/r/47992/ I decided not to change it because I couldn't forsee a case where startup logs would need to be other levels. e.g. You wouldn't log configuration at trace/error level. The only exception is WARN, but even then the startup log should be small enough to not warrant the separate level. So it didn't seem to be worth making the code more complicated. - Jake ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50583/#review143998 ----------------------------------------------------------- On July 28, 2016, 8:41 p.m., Jake Maes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50583/ > ----------------------------------------------------------- > > (Updated July 28, 2016, 8:41 p.m.) > > > Review request for samza, Boris Shkolnik, Chris Pettitt, Fred Ji, Jake Maes, > Navina Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data > Infrastructure). > > > Bugs: SAMZA-954 > https://issues.apache.org/jira/browse/SAMZA-954 > > > Repository: samza > > > Description > ------- > > SAMZA-954 Improve logging for Samza > > > Diffs > ----- > > docs/learn/documentation/versioned/jobs/logging.md > 0726d37affa06f85e20fbd3bc2395c28f30159a8 > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > a37f3536c45fdcb6d5410328f031b0263b71a82d > samza-core/src/main/scala/org/apache/samza/metrics/JmxServer.scala > ad00ca00f918df4d71d1c920b77027401a55c80b > samza-core/src/main/scala/org/apache/samza/util/Logging.scala > 250de1e2fa103be1a426d9da31187c12dbff8678 > samza-test/src/main/resources/log4j.xml > 29345f39ecef6f9ec769bf9d8eaab239f34e5d1e > > Diff: https://reviews.apache.org/r/50583/diff/ > > > Testing > ------- > > Tested using hello-samza with the following log4j.xml > > ``` > <!DOCTYPE log4j:configuration SYSTEM "log4j.dtd"> > <log4j:configuration xmlns:log4j="http://jakarta.apache.org/log4j/";> > <appender name="jmx" class="org.apache.samza.logging.log4j.JmxAppender" /> > > <appender name="RollingAppender" > class="org.apache.log4j.RollingFileAppender"> > <param name="File" value="${samza.log.dir}/${samza.container.name}.log" > /> > <param name="MaxFileSize" value="256MB" /> > <param name="MaxBackupIndex" value="20" /> > <layout class="org.apache.log4j.PatternLayout"> > <param name="ConversionPattern" value="%d{yyyy-MM-dd HH:mm:ss.SSS} [%t] > %c{1} [%p] %m%n" /> > </layout> > </appender> > <appender name="StartupAppender" > class="org.apache.log4j.RollingFileAppender"> > <param name="File" > value="${samza.log.dir}/${samza.container.name}-startup.log" /> > <param name="MaxFileSize" value="256MB" /> > <param name="MaxBackupIndex" value="1" /> > <layout class="org.apache.log4j.PatternLayout"> > <param name="ConversionPattern" value="%d{yyyy-MM-dd HH:mm:ss.SSS} [%t] > %c{1} [%p] %m%n" /> > </layout> > </appender> > <root> > <priority value="info" /> > <appender-ref ref="RollingAppender"/> > <appender-ref ref="jmx" /> > </root> > <logger name="STARTUP_LOGGER" additivity="false"> > <level value="info" /> > <appender-ref ref="StartupAppender"/> > </logger> > > </log4j:configuration> > > ``` > > > Thanks, > > Jake Maes > >
