----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18343/#review35520 -----------------------------------------------------------
Thanks for the updated patch, Jay! clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java <https://reviews.apache.org/r/18343/#comment66112> Should we have an equivalent trace message on start as well? clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java <https://reviews.apache.org/r/18343/#comment66113> Should this be log.trace("Requesting metadata update for topic {}", topic) clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java <https://reviews.apache.org/r/18343/#comment66114> Minor typo: ellapsed -> elapsed clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java <https://reviews.apache.org/r/18343/#comment66115> In addition to logging a successful send, I would expect trace to also log adding a message to the appropriate partition queue and also the attempt to send a request to a particular leader. clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/18343/#comment66116> I'm wondering why this is in debug? The rest of the places where we have a "sending" and a "sent" message, the "sending" message is at trace and the "sent" message is at debug clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/18343/#comment66117> Same here. Shouldn't this be at trace? We already have the debug statement for "Completed connection to node" clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/18343/#comment66118> In order to meaningfully trace a request in the producer, we need to include the correlation id and partition with every trace message for that request. It will be printed as part of the entire request but the grep will be more complex in this case. It is better to have it explicitly say "for partition {} with correlation id {}". clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/18343/#comment66119> It is useful to know the reason for a retry. Shouldn't this be at error? clients/src/main/java/org/apache/kafka/common/Cluster.java <https://reviews.apache.org/r/18343/#comment66120> what is the motivation for this change? - Neha Narkhede On Feb. 26, 2014, 5:32 a.m., Jay Kreps wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18343/ > ----------------------------------------------------------- > > (Updated Feb. 26, 2014, 5:32 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1250 > https://issues.apache.org/jira/browse/KAFKA-1250 > > > Repository: kafka > > > Description > ------- > > KAFKA-1250 Add logging to new producer. > > > Diffs > ----- > > build.gradle 58a6396f33413cc42231f445075f36552568563a > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java > e4bc97279585818860487a39a93b6481742b91db > clients/src/main/java/org/apache/kafka/clients/producer/ProducerRecord.java > 034bf33385fe3b4222f482ccd16de6b530e1c1a7 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java > 62613a3e29a7e5ffb1cc56d267793fef72857fc6 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java > ce5cf27efa08b79e501439cf79bc8666054a5429 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java > eb16f6d236e07b16654623606294a051531b5f58 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java > e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd > clients/src/main/java/org/apache/kafka/common/Cluster.java > 5caaaae1d2ad11dc9bb75008b066390a0dbe2afb > clients/src/main/java/org/apache/kafka/common/Node.java > 4197e5098c6556b22e9c56f385fcaa3604eda39e > clients/src/main/java/org/apache/kafka/common/PartitionInfo.java > 08d66f1a71fc5f1f11cfdc94f376c3f92c51e923 > clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java > c3148e5a9061d9748b617e3772c132fd89d35f05 > clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java > e08c349a6cd7617eb025709c3fb7890639ef610e > clients/src/main/java/org/apache/kafka/common/network/Selector.java > f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b > clients/src/main/java/org/apache/kafka/common/requests/RequestHeader.java > 457abb1ad44ed67ef6d015dc7859d1c4048d02fe > clients/src/main/java/org/apache/kafka/common/requests/RequestSend.java > c5e9020b2e7692c37c65bbe6bcbce5b37574baac > clients/src/main/java/org/apache/kafka/common/utils/KafkaThread.java > 9ff793f38d70d52580255d9c759c16e3c4116e10 > clients/src/main/java/org/apache/kafka/common/utils/Utils.java > 9c34e7dc82f33df7406cad0e64eb6a896d068dc6 > core/src/main/scala/kafka/server/ReplicaManager.scala > 3dd562ca93c44181bd6f75d4cbbd2669ea0d33aa > > Diff: https://reviews.apache.org/r/18343/diff/ > > > Testing > ------- > > > Thanks, > > Jay Kreps > >
