You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Ashish Singh <as...@cloudera.com> on 2015/05/01 00:53:27 UTC
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
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33614/#review82231
-----------------------------------------------------------
Ship it!
Ship It!
- Gwen Shapira
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
Posted by Ashish Singh <as...@cloudera.com>.
> 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
Posted by Gwen Shapira <gs...@cloudera.com>.
> 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
Posted by Aditya Auradkar <aa...@linkedin.com>.
-----------------------------------------------------------
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
Posted by Ashish Singh <as...@cloudera.com>.
> 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
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
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
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
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
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
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
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
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
Posted by Ashish Singh <as...@cloudera.com>.
> 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
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
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
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
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
Posted by Ashish Singh <as...@cloudera.com>.
> 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
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
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
>
>