You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Dong Lin <li...@gmail.com> on 2015/06/30 06:04:18 UTC
Review Request 36034: Patch for KAFKA-2306
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36034/
-----------------------------------------------------------
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
Posted by Dong Lin <li...@gmail.com>.
> 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.
>
> Guozhang Wang wrote:
> 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).
Yeah, that is my typo..
Thanks for the comment. Yes the "record-dropped-rate" is confusing. Could you please see if the updated patch addresses the issue? I used record-buffer-exhausted-rate since record-dropped-due-to-memory-exhausted-rate is probably too long.
- Dong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36034/#review90303
-----------------------------------------------------------
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
Posted by Guozhang Wang <wa...@gmail.com>.
> 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
Posted by Guozhang Wang <wa...@gmail.com>.
> 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
Posted by Dong Lin <li...@gmail.com>.
> 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
Posted by Dong Lin <li...@gmail.com>.
> 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
>
>
Re: Review Request 36034: Patch for KAFKA-2306
Posted by Dong Lin <li...@gmail.com>.
> 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
Posted by Guozhang Wang <wa...@gmail.com>.
-----------------------------------------------------------
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
>
>