----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28035/#review61461 -----------------------------------------------------------
Looks like this was coded against master. Are you planning to backport to 0.8.0 branch? I think we should either keep this module scala-free, or should add scala as a first-class citizen (i.e. implement the config code using real Scala). I'm not a big fan of bleeding Scala APIs into Java (e.g. Option). If we keep it scala free, we'll have to duplicate a couple of config names, but I think that's an OK trade-off. SAMZA-40 is going to fix this stuff (Config with Java as a first class citizen) anyway. build.gradle <https://reviews.apache.org/r/28035/#comment103120> If you keep scala, this will have to be a compile-time dependency, not testCompile. This is because Option is imported in the config object. samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java <https://reviews.apache.org/r/28035/#comment103094> Javadocs. samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java <https://reviews.apache.org/r/28035/#comment103092> Add to configuration-table.html samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java <https://reviews.apache.org/r/28035/#comment103096> Should try and guess the right default. If there's only one system defined, then we shouldn't require task.log4j.system to be set, we should just use the system. samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103080> Javadocs. samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103084> should probably private static final for all three of these. samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103085> private * for all four of these. samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103106> This is pretty confusing. Since we're using a string, can we just have it default to "application-master", and use "samza-container-%d" for containers? samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103088> Some logging in this method seems useful in cases where the appender can't properly instantiate itself. samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103089> need both job name and job id here to be fully unique. __samza-<job name>-<job id>-logs. Also, should string replace "-" to "_" for job name and job id. See: private def getTopic(jobName: String, jobId: String) = In KafkaCheckpointManagerFactory for an example. Might want to make this configurable, as well. Should still provide a default, but I can imagine use cases where folks might want to customize their stream name. samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103112> Can we do "samza-container-%d" here, just to make things a big more explicit. samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103116> Should call flushSystemProducer before stop. The SystemProducer implementations should do this as well, but just to be safe. samza-log4j/src/test/java/org/apache/samza/logging/log4j/TestSystemProducerAppender.java <https://reviews.apache.org/r/28035/#comment103077> License. - Chris Riccomini On Nov. 14, 2014, 11:40 a.m., Yan Fang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28035/ > ----------------------------------------------------------- > > (Updated Nov. 14, 2014, 11:40 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 > 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/logging/log4j/TestSystemProducerAppender.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/28035/diff/ > > > Testing > ------- > > > Thanks, > > Yan Fang > >
