> On Nov. 17, 2014, 7:19 p.m., Martin Kleppmann wrote: > > samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java, > > line 86 > > <https://reviews.apache.org/r/28035/diff/2/?file=764638#file764638line86> > > > > 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?)
they are used to set the streamName through the log4j.xml. such as <param name="StreamName" value="fooLog"/>. They are here for users who may do not want to use the default log stream name. > On Nov. 17, 2014, 7:19 p.m., Martin Kleppmann wrote: > > samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java, > > line 132 > > <https://reviews.apache.org/r/28035/diff/2/?file=764638#file764638line132> > > > > 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. yeah, I think this is a good add-on for the appender. Would prefer to open another ticket to keep track of that. - Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28035/#review61736 ----------------------------------------------------------- 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 > >
