----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47992/#review143409 -----------------------------------------------------------
Prioritized comments: Documentation is missing. Developers need to know when it's appropriate to use the startupLog() and users need to know that they should configure slf4j/log4j with a separate appender for that log. At a minimum there should be some javadoc, but I think some notes should be added and the example log4j.xml updated at http://samza.apache.org/learn/documentation/0.10/jobs/logging.html I would prefer a solution where existing users don't need to update their log4j settings to get the separate file, but I don't have an idea at the moment. The code looks ok. It's a little odd to infer the log level for a particular log method. It's possible that WARN level might be used for the startup log, but I don't see a scenario where ERROR is necessary. If an error occurs on startup, then presumably the primary logs won't grow large enough to lose the error. But the tradeoff makes the code simple, so I'm ok with it syntactically. - Jake Maes On July 20, 2016, 6:01 p.m., Jagadish Venkatraman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47992/ > ----------------------------------------------------------- > > (Updated July 20, 2016, 6:01 p.m.) > > > Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure). > > > Repository: samza > > > Description > ------- > > Often Samza jobs keep running for days (and in our case months). Log messages > generated by the job get rolled over. A lot of information that is critical > for debugging a job (like the jmx server port, URL, job model for the current > container, configuration that was passed to the container) is often printed > when the job starts. However, since logs get rolled over (by Yarn/ by > sys-administrators) when they exceed a certain size, this information is not > obvious. This RB adds a startup logger to the Logging trait. > > > Diffs > ----- > > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > cf3c4c0ab08a59760bc899c6f2027755e933b350 > samza-core/src/main/scala/org/apache/samza/metrics/JmxServer.scala > ad00ca00f918df4d71d1c920b77027401a55c80b > samza-core/src/main/scala/org/apache/samza/util/Logging.scala > 250de1e2fa103be1a426d9da31187c12dbff8678 > > Diff: https://reviews.apache.org/r/47992/diff/ > > > Testing > ------- > > <logger name="STARTUP" additivity="false"> > <priority value="info" /> > <appender-ref ref="startup_file_appender"/> > </logger> > > Verified log messages are correctly appended in both main, and startup logs. > > > Thanks, > > Jagadish Venkatraman > >
