Re: Review Request 47992: Improve logging by adding a startup logger.

[Jake Maes <ja...@gmail.com>]

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