> 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. > > Yan Fang wrote: > "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?
Totally. testCompile for Scala is fine with me. > 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? > > Yan Fang wrote: > 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. In Kafka's case, shouldn't it just work as-is with "application-master"? I think the default partitioner just hashCode's the object, then mod's it to the partition size. - Chris ----------------------------------------------------------- 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 > >
