Re: Review Request 33614: Patch for KAFKA-2132
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/#review89919 --- Thanks for the patch. Just a few minor comments below. build.gradle (lines 388 - 390) https://reviews.apache.org/r/33614/#comment142804 Could you explain a bit why we need this? build.gradle (lines 407 - 408) https://reviews.apache.org/r/33614/#comment142805 Since we already have a compile time dependency on these, we don't need to add them as test dependency. build.gradle (lines 411 - 414) https://reviews.apache.org/r/33614/#comment142807 Do we need to publish a test jar for kafka-log4j-appender? - Jun Rao On June 24, 2015, 5:25 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/ --- (Updated June 24, 2015, 5:25 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 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 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-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/MockKafkaLog4jAppender.java PRE-CREATION settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 Diff: https://reviews.apache.org/r/33614/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 33614: Patch for KAFKA-2132
On June 30, 2015, 5:12 p.m., Jun Rao wrote: Thanks for the patch. Just a few minor comments below. Thanks Jun for the review. On June 30, 2015, 5:12 p.m., Jun Rao wrote: build.gradle, lines 389-391 https://reviews.apache.org/r/33614/diff/9/?file=991359#file991359line389 Could you explain a bit why we need this? This is required as log4j-appender tests uses the MockSerializer, which is in org.apache.kafka.test. On June 30, 2015, 5:12 p.m., Jun Rao wrote: build.gradle, lines 408-409 https://reviews.apache.org/r/33614/diff/9/?file=991359#file991359line408 Since we already have a compile time dependency on these, we don't need to add them as test dependency. Got rid of slf4jlog4j. However, to be able to reuse MockSerializer only during testCompile time, dependency on clients' 'archives' config is required. On June 30, 2015, 5:12 p.m., Jun Rao wrote: build.gradle, lines 412-415 https://reviews.apache.org/r/33614/diff/9/?file=991359#file991359line412 Do we need to publish a test jar for kafka-log4j-appender? Not necessarily. Removed. - Ashish --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/#review89919 --- On June 24, 2015, 5:25 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/ --- (Updated June 24, 2015, 5:25 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 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 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-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/MockKafkaLog4jAppender.java PRE-CREATION settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 Diff: https://reviews.apache.org/r/33614/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 33614: Patch for KAFKA-2132
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/ --- (Updated June 30, 2015, 7:07 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 (updated) - build.gradle 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 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-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/MockKafkaLog4jAppender.java PRE-CREATION settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 Diff: https://reviews.apache.org/r/33614/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 33614: Patch for KAFKA-2132
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/ --- (Updated June 24, 2015, 5:20 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 (updated) - build.gradle 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 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-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/MockKafkaLog4jAppender.java PRE-CREATION settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 Diff: https://reviews.apache.org/r/33614/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 33614: Patch for KAFKA-2132
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/ --- (Updated June 24, 2015, 5:25 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 (updated) - build.gradle 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 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-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/MockKafkaLog4jAppender.java PRE-CREATION settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 Diff: https://reviews.apache.org/r/33614/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 33614: Patch for KAFKA-2132
On June 16, 2015, 1:31 a.m., Jun Rao wrote: Thanks for the patch. There seems to be a compilation error. :log4j-appender:compileTestJava /Users/junrao/intellij/kafka/log4j-appender/src/test/java/org/apache/kafka/log4jappender/MockKafkaLog4jAppender.java:27: cannot find symbol symbol : constructor MockProducer() location: class org.apache.kafka.clients.producer.MockProducer private MockProducer mockProducer = new MockProducer(); ^ Jun, thanks for the review. I had to rebase. It should be fine now. On June 16, 2015, 1:31 a.m., Jun Rao wrote: checkstyle/import-control.xml, line 98 https://reviews.apache.org/r/33614/diff/7/?file=984458#file984458line98 Shouldn't the subpackage be log4jappender? Yes, I am surprised that even log4j worked :) - Ashish --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/#review88015 --- On June 24, 2015, 5:20 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/ --- (Updated June 24, 2015, 5:20 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 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 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-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/MockKafkaLog4jAppender.java PRE-CREATION settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 Diff: https://reviews.apache.org/r/33614/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 33614: Patch for KAFKA-2132
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/#review88015 --- Thanks for the patch. There seems to be a compilation error. :log4j-appender:compileTestJava /Users/junrao/intellij/kafka/log4j-appender/src/test/java/org/apache/kafka/log4jappender/MockKafkaLog4jAppender.java:27: cannot find symbol symbol : constructor MockProducer() location: class org.apache.kafka.clients.producer.MockProducer private MockProducer mockProducer = new MockProducer(); ^ build.gradle https://reviews.apache.org/r/33614/#comment140421 It seems that this is also redundant? Could you fix it in this patch? checkstyle/import-control.xml https://reviews.apache.org/r/33614/#comment140419 Shouldn't the subpackage be log4jappender? log4j-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java https://reviews.apache.org/r/33614/#comment140420 Capitalize t in topic? - Jun Rao On June 14, 2015, 4:19 a.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/ --- (Updated June 14, 2015, 4:19 a.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 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 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-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/MockKafkaLog4jAppender.java PRE-CREATION settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 Diff: https://reviews.apache.org/r/33614/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 33614: Patch for KAFKA-2132
On June 11, 2015, 10:15 p.m., Aditya Auradkar wrote: log4j-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java, line 135 https://reviews.apache.org/r/33614/diff/6/?file=946526#file946526line135 perhaps wrap this inside an isDebugEnabled check? Ashish Singh wrote: That check is anyways done my the method itself, https://logging.apache.org/log4j/1.2/apidocs/org/apache/log4j/Category.html#debug(java.lang.Object). This is not Scala - so 'new Date(event.getTimeStamp())' will run before the method is called (no lazy eval), so lets skip the object creation unless we plan on logging. - Gwen --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/#review87638 --- On June 14, 2015, 4:19 a.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/ --- (Updated June 14, 2015, 4:19 a.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 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 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-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/MockKafkaLog4jAppender.java PRE-CREATION settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 Diff: https://reviews.apache.org/r/33614/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 33614: Patch for KAFKA-2132
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/ --- (Updated June 14, 2015, 4:19 a.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 (updated) - build.gradle 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 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-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/MockKafkaLog4jAppender.java PRE-CREATION settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 Diff: https://reviews.apache.org/r/33614/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 33614: Patch for KAFKA-2132
On June 12, 2015, 2:03 a.m., Jun Rao wrote: Thanks for the patch. A few comments below. Thanks for the review Jun. Addressed your comments. On June 12, 2015, 2:03 a.m., Jun Rao wrote: log4j-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java, lines 121-122 https://reviews.apache.org/r/33614/diff/6/?file=946526#file946526line121 Would it be better to use the built-in StringSerializer? Gwen also had the some comment :). Honestly, I had the same question. However, I did not want to change the original behaviour. I doubt using StringSerializer will change the behaviour, but having byte serializer is helpful for testing as it can be easily overridden by MockProducer. If we really have to use StringSerializer, I can create a wrapper on top of current MockProducer to support String values. On June 12, 2015, 2:03 a.m., Jun Rao wrote: build.gradle, line 219 https://reviews.apache.org/r/33614/diff/6/?file=946522#file946522line219 Could you check if this is needed? I think a compile dependency implies a testCompile dependency. Its not :). Removed. - Ashish --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/#review87670 --- On June 14, 2015, 4:19 a.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/ --- (Updated June 14, 2015, 4:19 a.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 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 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-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/MockKafkaLog4jAppender.java PRE-CREATION settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 Diff: https://reviews.apache.org/r/33614/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 33614: Patch for KAFKA-2132
On June 11, 2015, 10:15 p.m., Aditya Auradkar wrote: Hey Ashish, I've left a few minor comments. Thanks! Thanks for the review Aditya. Addressed your comments. On June 11, 2015, 10:15 p.m., Aditya Auradkar wrote: log4j-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java, line 135 https://reviews.apache.org/r/33614/diff/6/?file=946526#file946526line135 perhaps wrap this inside an isDebugEnabled check? That check is anyways done my the method itself, https://logging.apache.org/log4j/1.2/apidocs/org/apache/log4j/Category.html#debug(java.lang.Object). On June 11, 2015, 10:15 p.m., Aditya Auradkar wrote: log4j-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java, line 124 https://reviews.apache.org/r/33614/diff/6/?file=946526#file946526line124 can this be logged as info since it's infrequent? Based on Jun's comment, I am now using LogLog, which does not have an info level logging support. I also do not think it will be a blocker if this remains a debug level message. Let me know if you think otherwise. - Ashish --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/#review87638 --- On June 14, 2015, 4:19 a.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/ --- (Updated June 14, 2015, 4:19 a.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 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 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-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/MockKafkaLog4jAppender.java PRE-CREATION settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 Diff: https://reviews.apache.org/r/33614/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 33614: Patch for KAFKA-2132
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/#review87638 --- Hey Ashish, I've left a few minor comments. Thanks! log4j-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java https://reviews.apache.org/r/33614/#comment140037 Can you add some javadoc? log4j-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java https://reviews.apache.org/r/33614/#comment140032 can this be logged as info since it's infrequent? log4j-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java https://reviews.apache.org/r/33614/#comment140027 perhaps wrap this inside an isDebugEnabled check? log4j-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java https://reviews.apache.org/r/33614/#comment140029 can you change this to this.syncSend? log4j-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java https://reviews.apache.org/r/33614/#comment140028 perhaps you can use the ternary operator here? - Aditya Auradkar On April 30, 2015, 10:53 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/ --- (Updated April 30, 2015, 10:53 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 fef515b3b2276b1f861e7cc2e33e74c3ce5e405b 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-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/MockKafkaLog4jAppender.java PRE-CREATION settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 Diff: https://reviews.apache.org/r/33614/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 33614: Patch for KAFKA-2132
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/#review87670 --- Thanks for the patch. A few comments below. build.gradle https://reviews.apache.org/r/33614/#comment140087 Could you check if this is needed? I think a compile dependency implies a testCompile dependency. log4j-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java https://reviews.apache.org/r/33614/#comment140084 Should we use Logger inside log4j? Should we use LogLog as in the original scala code? log4j-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java https://reviews.apache.org/r/33614/#comment140085 Would it be better to use the built-in StringSerializer? log4j-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java https://reviews.apache.org/r/33614/#comment140086 We can just use ConfigException. - Jun Rao On April 30, 2015, 10:53 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/ --- (Updated April 30, 2015, 10:53 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 fef515b3b2276b1f861e7cc2e33e74c3ce5e405b 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-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/MockKafkaLog4jAppender.java PRE-CREATION settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 Diff: https://reviews.apache.org/r/33614/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 33614: Patch for KAFKA-2132
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/ --- (Updated April 30, 2015, 10:53 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 (updated) - build.gradle fef515b3b2276b1f861e7cc2e33e74c3ce5e405b 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-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java PRE-CREATION log4j-appender/src/test/java/org/apache/kafka/log4jappender/MockKafkaLog4jAppender.java PRE-CREATION settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 Diff: https://reviews.apache.org/r/33614/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 33614: Patch for KAFKA-2132
--- 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 006ced8d0c539d11e126aca62c147b916331 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
Re: Review Request 33614: Patch for KAFKA-2132
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/ --- (Updated April 28, 2015, 3 a.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 (updated) - build.gradle 006ced8d0c539d11e126aca62c147b916331 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
Re: Review Request 33614: Patch for KAFKA-2132
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33614/ --- (Updated April 28, 2015, 2:58 a.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 (updated) - build.gradle 006ced8d0c539d11e126aca62c147b916331 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
