> On Nov. 14, 2014, 7:05 p.m., Chris Riccomini wrote: > > 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.
"coded against master" -- yes, I realized it this morning :(. Let's see if I can get it done today, if not, I am ok with only committing to master. yes, i would prefere to make it scala-free because samza-log4j itself is pure-java. But in the test case, some Kafka APIs (such as "Utils.rm(server.config().logDirs())") only accept Scala time, is it ok to add scala in the test scope? > On Nov. 14, 2014, 7:05 p.m., Chris Riccomini wrote: > > samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java, > > line 54 > > <https://reviews.apache.org/r/28035/diff/1/?file=763402#file763402line54> > > > > 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? yes, I think we can. Then it will be the system's responsiblity to partition based on the key. For example, in the kafka, users need to write a customized partition class to deal with "application-master". Another option is that, we can have partitionKey and Key. Use "0, 1, ..." for partitionKey and "application-master" for key. > On Nov. 14, 2014, 7:05 p.m., Chris Riccomini wrote: > > samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java, > > line 72 > > <https://reviews.apache.org/r/28035/diff/1/?file=763402#file763402line72> > > > > 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. agree. especially when you have to create the topic in the kafka with appropriate partition number. - Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28035/#review61461 ----------------------------------------------------------- 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 > >
