Re: Review Request 36034: Patch for KAFKA-2306
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/#review90712 --- Ship it! Ship It! - Guozhang Wang On July 7, 2015, 1:22 a.m., Dong Lin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/ --- (Updated July 7, 2015, 1:22 a.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-2306 https://issues.apache.org/jira/browse/KAFKA-2306 Repository: kafka Description --- KAFKA-2306; New producer should emit metrics for buffer exhaustion Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java 87dbd64f30f35dbf31d3820f9819a63c6c0d1e58 Diff: https://reviews.apache.org/r/36034/diff/ Testing --- Thanks, Dong Lin
Re: Review Request 36034: Patch for KAFKA-2306
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/ --- (Updated July 7, 2015, 5:47 p.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-2306 https://issues.apache.org/jira/browse/KAFKA-2306 Repository: kafka Description --- KAFKA-2306; New producer should emit metrics for buffer exhaustion Diffs (updated) - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java 87dbd64f30f35dbf31d3820f9819a63c6c0d1e58 Diff: https://reviews.apache.org/r/36034/diff/ Testing --- Thanks, Dong Lin
Re: Review Request 36034: Patch for KAFKA-2306
On July 7, 2015, 5:28 p.m., Guozhang Wang wrote: clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, line 408 https://reviews.apache.org/r/36034/diff/3/?file=1000835#file1000835line408 Missed one more point here: I think it is better to use buffer-exhausted-records as the sensor name just to be consistent with ther sensor names. What do you think? Sure. Thanks much for catching that. I forgot to check the name of other sensors.. - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/#review90730 --- On July 7, 2015, 5:47 p.m., Dong Lin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/ --- (Updated July 7, 2015, 5:47 p.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-2306 https://issues.apache.org/jira/browse/KAFKA-2306 Repository: kafka Description --- KAFKA-2306; New producer should emit metrics for buffer exhaustion Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java 87dbd64f30f35dbf31d3820f9819a63c6c0d1e58 Diff: https://reviews.apache.org/r/36034/diff/ Testing --- Thanks, Dong Lin
Re: Review Request 36034: Patch for KAFKA-2306
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/#review90765 --- Ship it! Ship It! - Guozhang Wang On July 7, 2015, 5:47 p.m., Dong Lin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/ --- (Updated July 7, 2015, 5:47 p.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-2306 https://issues.apache.org/jira/browse/KAFKA-2306 Repository: kafka Description --- KAFKA-2306; New producer should emit metrics for buffer exhaustion Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java 87dbd64f30f35dbf31d3820f9819a63c6c0d1e58 Diff: https://reviews.apache.org/r/36034/diff/ Testing --- Thanks, Dong Lin
Re: Review Request 36034: Patch for KAFKA-2306
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/#review90730 --- clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java (line 408) https://reviews.apache.org/r/36034/#comment143865 Missed one more point here: I think it is better to use buffer-exhausted-records as the sensor name just to be consistent with ther sensor names. What do you think? - Guozhang Wang On July 7, 2015, 1:22 a.m., Dong Lin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/ --- (Updated July 7, 2015, 1:22 a.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-2306 https://issues.apache.org/jira/browse/KAFKA-2306 Repository: kafka Description --- KAFKA-2306; New producer should emit metrics for buffer exhaustion Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java 87dbd64f30f35dbf31d3820f9819a63c6c0d1e58 Diff: https://reviews.apache.org/r/36034/diff/ Testing --- Thanks, Dong Lin
Re: Review Request 36034: Patch for KAFKA-2306
On July 3, 2015, 1:36 a.m., Guozhang Wang wrote: Thanks for the patch. I have a few thoughts regarding the names of the metrics, since in the producer other causes can also result in dropped messages (i.e. rejected before it enteres the producer buffer), such as message-size-too-large, serialization-failed, etc. In the old producer since we only have one cause we named that to droppedMessageRate. So I think we could either: 1. record dropped messages for any KafkaExceptions, but not limited to BufferExhaustedException. 2. have a separate metric for buffer-exhausted with a different name. I prefer the first option since I feel in practice people just want to distinguish between the case that messages failed to get into the producer from the case the messages gets failed to send to the broker. Dong Lin wrote: Thanks for your comments. If I understand it right, we already have a sensor named errors which records the message dropped for any KafkaExceptions. Therefore no work is needed option 1. I think there is probably need in opertaion to track the number of messages dropped due to BufferExhaustedException. Because 1) this is a common exception may happen un-noticed if block_on_buffer_full=false and asynchronous producer is used; and 2) for backward compatibility with old producer. I will ask Joel if he has more detail on the motivation. Guozhang Wang wrote: There are two cenarios that a message does not successfully reach the broker: 1. The message gets rejected by the producer immediately before being added to the producer's buffer for sending. This error is thrown as non-ApiException KafkaException. 2. The message was sent to the broker from the producer's send buffer but got rejected (and later retries exhausted). This error is thrown as ApiException. Currently both of them are recorded as record-error-rate, while in the old producer, we record DroppedRecord for the first scenario, which only includes BufferFullException. So I was proposing if we want back-ward compatibility we could record the first scenario, which include BufferExhausted but also some other exceptions in a separate metric. Does that sound reasonable? Dong Lin wrote: Yeah I understand your suggestion. But could you please explain why having a sensor for all non-ApiException KafkaException is better than ApiException KafkaException only? I think it wouldn't be exactly backward compatible if you include other exceptions, such as ClassCastException, in this sensor. It could cause problem to users if they are depending on this sensor to measure how many data are dropped in asynchronous call due to BufferFullException. What do you think? Dong Lin wrote: oops. I mean SerializationException, not ClassCastException, in the comment above. BTW, the addition of this sensor is motivated when we prepare the release note of new producer. I think if this sensor is not important, we don't need to add it. Otherwise, it is easier to explain to user with its original definition. I think you meant to say why having a sensor for all non-ApiException KafkaException is better than BufferExhaustedException only, right? I do not preferring to having a metric for all non-ApiExceptions, as I said in the first comment, we could do this (as option 1) or use a different name as record-dropped-rate since it is too general as referring to any non-ApiException that cause it to drop messages, for example record-dropped-due-to-memory-exhausted-rate for BufferExhaustedException only if users just want that (as option 2). - Guozhang --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/#review90303 --- On July 3, 2015, 1:56 a.m., Dong Lin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/ --- (Updated July 3, 2015, 1:56 a.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-2306 https://issues.apache.org/jira/browse/KAFKA-2306 Repository: kafka Description --- KAFKA-2306; New producer should emit metrics for buffer exhaustion Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 0baf16e55046a2f49f6431e01d52c323c95eddf0 Diff: https://reviews.apache.org/r/36034/diff/ Testing --- Thanks, Dong Lin
Re: Review Request 36034: Patch for KAFKA-2306
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/#review90614 --- clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java (line 254) https://reviews.apache.org/r/36034/#comment143728 I think we do not need to create a varialbe here since it is only referenced once in the code. Instead we could just call the following in line 410: metrics.sensor(...).record(); clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java (lines 399 - 402) https://reviews.apache.org/r/36034/#comment143726 This metric may better be added in RecordAccumulator's registerMetrics() rather than in Sender. Also, the naming convention is not aligned with others, would better be buffer-exhausted-rate. - Guozhang Wang On July 6, 2015, 9:54 p.m., Dong Lin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/ --- (Updated July 6, 2015, 9:54 p.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-2306 https://issues.apache.org/jira/browse/KAFKA-2306 Repository: kafka Description --- KAFKA-2306; New producer should emit metrics for buffer exhaustion Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 0baf16e55046a2f49f6431e01d52c323c95eddf0 Diff: https://reviews.apache.org/r/36034/diff/ Testing --- Thanks, Dong Lin
Re: Review Request 36034: Patch for KAFKA-2306
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/ --- (Updated July 6, 2015, 9:54 p.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-2306 https://issues.apache.org/jira/browse/KAFKA-2306 Repository: kafka Description --- KAFKA-2306; New producer should emit metrics for buffer exhaustion Diffs (updated) - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 0baf16e55046a2f49f6431e01d52c323c95eddf0 Diff: https://reviews.apache.org/r/36034/diff/ Testing --- Thanks, Dong Lin
Re: Review Request 36034: Patch for KAFKA-2306
On July 3, 2015, 1:36 a.m., Guozhang Wang wrote: Thanks for the patch. I have a few thoughts regarding the names of the metrics, since in the producer other causes can also result in dropped messages (i.e. rejected before it enteres the producer buffer), such as message-size-too-large, serialization-failed, etc. In the old producer since we only have one cause we named that to droppedMessageRate. So I think we could either: 1. record dropped messages for any KafkaExceptions, but not limited to BufferExhaustedException. 2. have a separate metric for buffer-exhausted with a different name. I prefer the first option since I feel in practice people just want to distinguish between the case that messages failed to get into the producer from the case the messages gets failed to send to the broker. Dong Lin wrote: Thanks for your comments. If I understand it right, we already have a sensor named errors which records the message dropped for any KafkaExceptions. Therefore no work is needed option 1. I think there is probably need in opertaion to track the number of messages dropped due to BufferExhaustedException. Because 1) this is a common exception may happen un-noticed if block_on_buffer_full=false and asynchronous producer is used; and 2) for backward compatibility with old producer. I will ask Joel if he has more detail on the motivation. Guozhang Wang wrote: There are two cenarios that a message does not successfully reach the broker: 1. The message gets rejected by the producer immediately before being added to the producer's buffer for sending. This error is thrown as non-ApiException KafkaException. 2. The message was sent to the broker from the producer's send buffer but got rejected (and later retries exhausted). This error is thrown as ApiException. Currently both of them are recorded as record-error-rate, while in the old producer, we record DroppedRecord for the first scenario, which only includes BufferFullException. So I was proposing if we want back-ward compatibility we could record the first scenario, which include BufferExhausted but also some other exceptions in a separate metric. Does that sound reasonable? Yeah I understand your suggestion. But could you please explain why having a sensor for all non-ApiException KafkaException is better than ApiException KafkaException only? I think it wouldn't be exactly backward compatible if you include other exceptions, such as ClassCastException, in this sensor. It could cause problem to users if they are depending on this sensor to measure how many data are dropped in asynchronous call due to BufferFullException. What do you think? - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/#review90303 --- On July 3, 2015, 1:56 a.m., Dong Lin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/ --- (Updated July 3, 2015, 1:56 a.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-2306 https://issues.apache.org/jira/browse/KAFKA-2306 Repository: kafka Description --- KAFKA-2306; New producer should emit metrics for buffer exhaustion Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 0baf16e55046a2f49f6431e01d52c323c95eddf0 Diff: https://reviews.apache.org/r/36034/diff/ Testing --- Thanks, Dong Lin
Re: Review Request 36034: Patch for KAFKA-2306
On July 3, 2015, 1:36 a.m., Guozhang Wang wrote: Thanks for the patch. I have a few thoughts regarding the names of the metrics, since in the producer other causes can also result in dropped messages (i.e. rejected before it enteres the producer buffer), such as message-size-too-large, serialization-failed, etc. In the old producer since we only have one cause we named that to droppedMessageRate. So I think we could either: 1. record dropped messages for any KafkaExceptions, but not limited to BufferExhaustedException. 2. have a separate metric for buffer-exhausted with a different name. I prefer the first option since I feel in practice people just want to distinguish between the case that messages failed to get into the producer from the case the messages gets failed to send to the broker. Dong Lin wrote: Thanks for your comments. If I understand it right, we already have a sensor named errors which records the message dropped for any KafkaExceptions. Therefore no work is needed option 1. I think there is probably need in opertaion to track the number of messages dropped due to BufferExhaustedException. Because 1) this is a common exception may happen un-noticed if block_on_buffer_full=false and asynchronous producer is used; and 2) for backward compatibility with old producer. I will ask Joel if he has more detail on the motivation. Guozhang Wang wrote: There are two cenarios that a message does not successfully reach the broker: 1. The message gets rejected by the producer immediately before being added to the producer's buffer for sending. This error is thrown as non-ApiException KafkaException. 2. The message was sent to the broker from the producer's send buffer but got rejected (and later retries exhausted). This error is thrown as ApiException. Currently both of them are recorded as record-error-rate, while in the old producer, we record DroppedRecord for the first scenario, which only includes BufferFullException. So I was proposing if we want back-ward compatibility we could record the first scenario, which include BufferExhausted but also some other exceptions in a separate metric. Does that sound reasonable? Dong Lin wrote: Yeah I understand your suggestion. But could you please explain why having a sensor for all non-ApiException KafkaException is better than ApiException KafkaException only? I think it wouldn't be exactly backward compatible if you include other exceptions, such as ClassCastException, in this sensor. It could cause problem to users if they are depending on this sensor to measure how many data are dropped in asynchronous call due to BufferFullException. What do you think? oops. I mean SerializationException, not ClassCastException, in the comment above. BTW, the addition of this sensor is motivated when we prepare the release note of new producer. I think if this sensor is not important, we don't need to add it. Otherwise, it is easier to explain to user with its original definition. - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/#review90303 --- On July 3, 2015, 1:56 a.m., Dong Lin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/ --- (Updated July 3, 2015, 1:56 a.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-2306 https://issues.apache.org/jira/browse/KAFKA-2306 Repository: kafka Description --- KAFKA-2306; New producer should emit metrics for buffer exhaustion Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 0baf16e55046a2f49f6431e01d52c323c95eddf0 Diff: https://reviews.apache.org/r/36034/diff/ Testing --- Thanks, Dong Lin
Re: Review Request 36034: Patch for KAFKA-2306
On July 3, 2015, 1:36 a.m., Guozhang Wang wrote: Thanks for the patch. I have a few thoughts regarding the names of the metrics, since in the producer other causes can also result in dropped messages (i.e. rejected before it enteres the producer buffer), such as message-size-too-large, serialization-failed, etc. In the old producer since we only have one cause we named that to droppedMessageRate. So I think we could either: 1. record dropped messages for any KafkaExceptions, but not limited to BufferExhaustedException. 2. have a separate metric for buffer-exhausted with a different name. I prefer the first option since I feel in practice people just want to distinguish between the case that messages failed to get into the producer from the case the messages gets failed to send to the broker. Dong Lin wrote: Thanks for your comments. If I understand it right, we already have a sensor named errors which records the message dropped for any KafkaExceptions. Therefore no work is needed option 1. I think there is probably need in opertaion to track the number of messages dropped due to BufferExhaustedException. Because 1) this is a common exception may happen un-noticed if block_on_buffer_full=false and asynchronous producer is used; and 2) for backward compatibility with old producer. I will ask Joel if he has more detail on the motivation. There are two cenarios that a message does not successfully reach the broker: 1. The message gets rejected by the producer immediately before being added to the producer's buffer for sending. This error is thrown as non-ApiException KafkaException. 2. The message was sent to the broker from the producer's send buffer but got rejected (and later retries exhausted). This error is thrown as ApiException. Currently both of them are recorded as record-error-rate, while in the old producer, we record DroppedRecord for the first scenario, which only includes BufferFullException. So I was proposing if we want back-ward compatibility we could record the first scenario, which include BufferExhausted but also some other exceptions in a separate metric. Does that sound reasonable? - Guozhang --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/#review90303 --- On July 3, 2015, 1:56 a.m., Dong Lin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/ --- (Updated July 3, 2015, 1:56 a.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-2306 https://issues.apache.org/jira/browse/KAFKA-2306 Repository: kafka Description --- KAFKA-2306; New producer should emit metrics for buffer exhaustion Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 0baf16e55046a2f49f6431e01d52c323c95eddf0 Diff: https://reviews.apache.org/r/36034/diff/ Testing --- Thanks, Dong Lin
Re: Review Request 36034: Patch for KAFKA-2306
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/#review90303 --- Thanks for the patch. I have a few thoughts regarding the names of the metrics, since in the producer other causes can also result in dropped messages (i.e. rejected before it enteres the producer buffer), such as message-size-too-large, serialization-failed, etc. In the old producer since we only have one cause we named that to droppedMessageRate. So I think we could either: 1. record dropped messages for any KafkaExceptions, but not limited to BufferExhaustedException. 2. have a separate metric for buffer-exhausted with a different name. I prefer the first option since I feel in practice people just want to distinguish between the case that messages failed to get into the producer from the case the messages gets failed to send to the broker. - Guozhang Wang On June 30, 2015, 4:04 a.m., Dong Lin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/ --- (Updated June 30, 2015, 4:04 a.m.) Review request for kafka. Bugs: KAFKA-2306 https://issues.apache.org/jira/browse/KAFKA-2306 Repository: kafka Description --- KAFKA-2306; New producer should emit metrics for buffer exhaustion Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 0baf16e55046a2f49f6431e01d52c323c95eddf0 Diff: https://reviews.apache.org/r/36034/diff/ Testing --- Thanks, Dong Lin
Re: Review Request 36034: Patch for KAFKA-2306
On July 3, 2015, 1:36 a.m., Guozhang Wang wrote: Thanks for the patch. I have a few thoughts regarding the names of the metrics, since in the producer other causes can also result in dropped messages (i.e. rejected before it enteres the producer buffer), such as message-size-too-large, serialization-failed, etc. In the old producer since we only have one cause we named that to droppedMessageRate. So I think we could either: 1. record dropped messages for any KafkaExceptions, but not limited to BufferExhaustedException. 2. have a separate metric for buffer-exhausted with a different name. I prefer the first option since I feel in practice people just want to distinguish between the case that messages failed to get into the producer from the case the messages gets failed to send to the broker. Thanks for your comments. If I understand it right, we already have a sensor named errors which records the message dropped for any KafkaExceptions. Therefore no work is needed option 1. I think there is probably need in opertaion to track the number of messages dropped due to BufferExhaustedException. Because 1) this is a common exception may happen un-noticed if block_on_buffer_full=false and asynchronous producer is used; and 2) for backward compatibility with old producer. I will ask Joel if he has more detail on the motivation. - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/#review90303 --- On June 30, 2015, 4:04 a.m., Dong Lin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36034/ --- (Updated June 30, 2015, 4:04 a.m.) Review request for kafka. Bugs: KAFKA-2306 https://issues.apache.org/jira/browse/KAFKA-2306 Repository: kafka Description --- KAFKA-2306; New producer should emit metrics for buffer exhaustion Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 0baf16e55046a2f49f6431e01d52c323c95eddf0 Diff: https://reviews.apache.org/r/36034/diff/ Testing --- Thanks, Dong Lin
