----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28035/#review61736 -----------------------------------------------------------
samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java <https://reviews.apache.org/r/28035/#comment103618> For ShellCommandConfig.ENV_CONFIG and ShellCommandConfig.ENV_COORDINATOR_URL (below) you call through to the Scala classes in samza-core, whereas here you duplicate the properties. Can you do the same here, to avoid the duplication? samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103605> Suggestion: how about calling it StreamAppender (or StreamLogAppender)? That would make its purpose more apparent to a user (it sends the logs to a stream). SystemProducerAppender is really a description of how it's implemented, not what it does. samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103615> getConfig() is being called twice. samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103616> How could streamName be non-null? setStreamName is never called. (Also, getStreamName and setStreamName could probably be removed, or are they there for a purpose?) samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103645> Should also call systemProducer.start(). Even though for KafkaSystemProducer that's a no-op, other systems may require it. samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103626> This log message is meaningless for someone who hasn't read the code. I'd suggest coalescing all the "log.info"s in this method into a single call that summarises the log configuration with a useful message. samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103607> Nitpick: strange line breaking (you might as well put the "UTF-8" on the previous line). samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103635> Did you mean to append two strings without a space? Or did you want to pass the exception as a second argument? samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103608> Wishlist item: do you think it would be possible to move the encoding of the LoggingEvent to a serde? I think this string encoding is a fine default, but some users may have a structured log format that they want to use. For example, LinkedIn has an Avro schema for logs, which includes not just the log message (as a string) but also metadata like the timestamp, the host it was running on, the job name and suchlike. If a custom serde could be plugged in, it would be a natural place for generating such structured log messages. However, I don't want to add further scope creep to this issue, so I'm fine if we just leave it as it is for now. We can always revisit that later. samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103613> Nitpick: would be easier to read without line break. (http://samza.incubator.apache.org/contribute/coding-guide.html says we're not limited to 80-character lines) samza-log4j/src/test/java/org/apache/samza/logging/log4j/TestSystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103665> It seems a bit heavyweight to me to test the Log4j appender by firing up Zookeeper and Kafka. This is really testing the samza-kafka module more than it's testing samza-log4j, and will further increase the time it takes to run the tests. Perhaps it would be sufficient to test samza-log4j with a mock system (rather than a real Kafka system)? samza-log4j/src/test/java/org/apache/samza/logging/log4j/TestSystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103659> systemTime is an odd name for a variable that is actually mock time. samza-log4j/src/test/java/org/apache/samza/logging/log4j/TestSystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103657> Would be more conventional for a class to start with an uppercase letter. - Martin Kleppmann On Nov. 15, 2014, 12:24 a.m., Yan Fang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28035/ > ----------------------------------------------------------- > > (Updated Nov. 15, 2014, 12:24 a.m.) > > > Review request for samza. > > > Bugs: SAMZA-310 > https://issues.apache.org/jira/browse/SAMZA-310 > > > Repository: samza > > > Description > ------- > > Added a log4j appender using SystemProducer > Added a log4jConfig class to help get log4j specific config > Unit test code > > > Diffs > ----- > > build.gradle 828bce9 > docs/learn/documentation/versioned/jobs/configuration-table.html fbb5ea4 > docs/learn/documentation/versioned/jobs/logging.md 58e56c1 > samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java > PRE-CREATION > > samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java > PRE-CREATION > > samza-log4j/src/test/java/org/apache/samza/config/TestLog4jSystemConfig.java > PRE-CREATION > > samza-log4j/src/test/java/org/apache/samza/logging/log4j/TestSystemProducerAppender.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/28035/diff/ > > > Testing > ------- > > > Thanks, > > Yan Fang > >
