----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50583/#review143998 -----------------------------------------------------------
Ship it! One minor suggestion below. samza-core/src/main/scala/org/apache/samza/util/Logging.scala (line 31) <https://reviews.apache.org/r/50583/#comment209931> 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. - Xinyu Liu 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 > >
