----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/#review82056 -----------------------------------------------------------
Overall, looks good. I had a bunch of nits :) build.gradle <https://reviews.apache.org/r/33614/#comment132709> nit: log4j may be a bit confusing. can we call it log4j-producer or log4j-appender? build.gradle <https://reviews.apache.org/r/33614/#comment132710> What's this? log4j/src/main/java/org/apache/kafka/log4j/KafkaLog4jAppender.java <https://reviews.apache.org/r/33614/#comment132711> extra newline? log4j/src/main/java/org/apache/kafka/log4j/KafkaLog4jAppender.java <https://reviews.apache.org/r/33614/#comment132712> extra newline log4j/src/main/java/org/apache/kafka/log4j/KafkaLog4jAppender.java <https://reviews.apache.org/r/33614/#comment132713> extra newline... sorry for nitpicks. just some cleanup :) log4j/src/main/java/org/apache/kafka/log4j/KafkaLog4jAppender.java <https://reviews.apache.org/r/33614/#comment132714> newline log4j/src/main/java/org/apache/kafka/log4j/KafkaLog4jAppender.java <https://reviews.apache.org/r/33614/#comment132715> Kafka project doesn't use brackets for single line "if", so: if (props.inEmpty()) throw new MissingConfigException... log4j/src/main/java/org/apache/kafka/log4j/KafkaLog4jAppender.java <https://reviews.apache.org/r/33614/#comment132716> Any idea why we are using ByteArraySerializer (and not StringSerializer) when the messages are strings? I know the original class was the same, but I'm not sure why. log4j/src/test/java/org/apache/kafka/log4j/KafkaLog4jAppenderTest.java <https://reviews.apache.org/r/33614/#comment132721> nice :) - Gwen Shapira On April 28, 2015, 9:38 p.m., Ashish Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33614/ > ----------------------------------------------------------- > > (Updated April 28, 2015, 9:38 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2132 > https://issues.apache.org/jira/browse/KAFKA-2132 > > > Repository: kafka > > > Description > ------- > > KAFKA-2132: Move Log4J appender to clients module > > > Diffs > ----- > > build.gradle 006ced44448d0c539d11e126aca62c147b916331 > checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03 > core/src/main/scala/kafka/producer/KafkaLog4jAppender.scala > 5d36a019e3dbfb93737a9cd23404dcd1c5d836d1 > core/src/test/scala/unit/kafka/log4j/KafkaLog4jAppenderTest.scala > 41366a14590d318fced0e83d6921d8035fa882da > log4j/src/main/java/org/apache/kafka/log4j/KafkaLog4jAppender.java > PRE-CREATION > log4j/src/test/java/org/apache/kafka/log4j/KafkaLog4jAppenderTest.java > PRE-CREATION > log4j/src/test/java/org/apache/kafka/log4j/MockKafkaLog4jAppender.java > PRE-CREATION > settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 > > Diff: https://reviews.apache.org/r/33614/diff/ > > > Testing > ------- > > > Thanks, > > Ashish Singh > >
