You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Aditya Auradkar <aa...@linkedin.com> on 2015/06/12 00:15:01 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/#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 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
>
>