You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jay Kreps <bo...@gmail.com> on 2014/02/07 05:23:38 UTC
Review Request 17836: Better error message for underflow during struct
deserialization
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17836/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1241
https://issues.apache.org/jira/browse/KAFKA-1241
Repository: kafka
Description
-------
KAFKA-1241 Better error message for underflow on read of struct.
Diffs
-----
clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java 0c11a04e121904b32673b0a86739e02f03066483
clients/src/main/java/org/apache/kafka/common/protocol/types/Type.java 24ac060a82767f913dc926ccfe99d83a596e8cf3
Diff: https://reviews.apache.org/r/17836/diff/
Testing
-------
Thanks,
Jay Kreps
Re: Review Request 17836: Better error message for underflow during struct
deserialization
Posted by Neha Narkhede <ne...@gmail.com>.
> On Feb. 7, 2014, 4:29 a.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java, line 53
> > <https://reviews.apache.org/r/17836/diff/1/?file=479011#file479011line53>
> >
> > This will state the field correctly. One other outstanding issue is stating the request type correctly. Would this fix that as well?
>
> Jay Kreps wrote:
> No, as I described in the JIRA the serialization code is just about reading and writing defined serialized types it doesn't and definitely shouldn't know that there is such a thing as requests.
>
> Neha Narkhede wrote:
> Understand that we don't have all the fixes now. I guess what I was saying is that it is very useful to have better error reporting in a new code base in order to understand bugs. How large is the fix to get the serde to report the right request type?
>
> Jay Kreps wrote:
> I think I'm not being clear. The serialization layer is like Avro. It doesn't log. It definitely doesn't log about kafka requests! It doesn't know about kafka requests for the same reason Avro doesn't know about kafka requests! It just knows about serialization--i.e. turning a schema and object into bytes and vice versa.
>
> We can definitely improve logging in the Sender when we add that but please please not in the serialization library!
Sure, my suggestion was to find a way to fix it somewhere in the client, not the serialization library. Understand why that shouldn't know anything about kafka requests.
> On Feb. 7, 2014, 4:29 a.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java, line 52
> > <https://reviews.apache.org/r/17836/diff/1/?file=479011#file479011line52>
> >
> > Does it make sense to catch all Throwables so we get meaningful error message in all cases?
>
> Jay Kreps wrote:
> No, catching Throwable is a shifty thing to do. Throwable is the union of application errors (Exception) and unrecoverable errors (OOM etc). Catching the later and calling it a SchemaException would not be good.
>
> Neha Narkhede wrote:
> Hmm.. how about catching the rest of the unrecoverable errors and at least logging a meaningful error message for those, before rethrowing those as is (which is what will happen anyways if the code hits an unrecoverable error). I just think that hitting these errors due to bugs or whatever and not having a clue of what happened is kind of annoying.
>
> Jay Kreps wrote:
> I think that is not a good idea:
> http://stackoverflow.com/questions/581878/why-catch-exceptions-in-java-when-you-can-catch-throwables
> We catch the Exceptions and wrap them for a good error, this should cover virtually all application bugs etc (which might actually be too much, e.g. do we really want to catch NullPointerException). The only thing that catching Throwable does is add to this java.lang.Errors. From the javadoc on Errors:
> "An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch."
I see, this probably needs discussion as currently we catch throwables in a number of places in Kafka. The sole purpose of that is to leave an understandable log behind to allow us to fix the issue.
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17836/#review33898
-----------------------------------------------------------
On Feb. 7, 2014, 4:23 a.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17836/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2014, 4:23 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1241
> https://issues.apache.org/jira/browse/KAFKA-1241
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1241 Better error message for underflow on read of struct.
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java 0c11a04e121904b32673b0a86739e02f03066483
> clients/src/main/java/org/apache/kafka/common/protocol/types/Type.java 24ac060a82767f913dc926ccfe99d83a596e8cf3
>
> Diff: https://reviews.apache.org/r/17836/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 17836: Better error message for underflow during struct
deserialization
Posted by Jay Kreps <bo...@gmail.com>.
> On Feb. 7, 2014, 4:29 a.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java, line 52
> > <https://reviews.apache.org/r/17836/diff/1/?file=479011#file479011line52>
> >
> > Does it make sense to catch all Throwables so we get meaningful error message in all cases?
No, catching Throwable is a shifty thing to do. Throwable is the union of application errors (Exception) and unrecoverable errors (OOM etc). Catching the later and calling it a SchemaException would not be good.
> On Feb. 7, 2014, 4:29 a.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java, line 53
> > <https://reviews.apache.org/r/17836/diff/1/?file=479011#file479011line53>
> >
> > This will state the field correctly. One other outstanding issue is stating the request type correctly. Would this fix that as well?
No, as I described in the JIRA the serialization code is just about reading and writing defined serialized types it doesn't and definitely shouldn't know that there is such a thing as requests.
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17836/#review33898
-----------------------------------------------------------
On Feb. 7, 2014, 4:23 a.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17836/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2014, 4:23 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1241
> https://issues.apache.org/jira/browse/KAFKA-1241
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1241 Better error message for underflow on read of struct.
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java 0c11a04e121904b32673b0a86739e02f03066483
> clients/src/main/java/org/apache/kafka/common/protocol/types/Type.java 24ac060a82767f913dc926ccfe99d83a596e8cf3
>
> Diff: https://reviews.apache.org/r/17836/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 17836: Better error message for underflow during struct
deserialization
Posted by Neha Narkhede <ne...@gmail.com>.
> On Feb. 7, 2014, 4:29 a.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java, line 52
> > <https://reviews.apache.org/r/17836/diff/1/?file=479011#file479011line52>
> >
> > Does it make sense to catch all Throwables so we get meaningful error message in all cases?
>
> Jay Kreps wrote:
> No, catching Throwable is a shifty thing to do. Throwable is the union of application errors (Exception) and unrecoverable errors (OOM etc). Catching the later and calling it a SchemaException would not be good.
Hmm.. how about catching the rest of the unrecoverable errors and at least logging a meaningful error message for those, before rethrowing those as is (which is what will happen anyways if the code hits an unrecoverable error). I just think that hitting these errors due to bugs or whatever and not having a clue of what happened is kind of annoying.
> On Feb. 7, 2014, 4:29 a.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java, line 53
> > <https://reviews.apache.org/r/17836/diff/1/?file=479011#file479011line53>
> >
> > This will state the field correctly. One other outstanding issue is stating the request type correctly. Would this fix that as well?
>
> Jay Kreps wrote:
> No, as I described in the JIRA the serialization code is just about reading and writing defined serialized types it doesn't and definitely shouldn't know that there is such a thing as requests.
Understand that we don't have all the fixes now. I guess what I was saying is that it is very useful to have better error reporting in a new code base in order to understand bugs. How large is the fix to get the serde to report the right request type?
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17836/#review33898
-----------------------------------------------------------
On Feb. 7, 2014, 4:23 a.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17836/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2014, 4:23 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1241
> https://issues.apache.org/jira/browse/KAFKA-1241
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1241 Better error message for underflow on read of struct.
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java 0c11a04e121904b32673b0a86739e02f03066483
> clients/src/main/java/org/apache/kafka/common/protocol/types/Type.java 24ac060a82767f913dc926ccfe99d83a596e8cf3
>
> Diff: https://reviews.apache.org/r/17836/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 17836: Better error message for underflow during struct
deserialization
Posted by Jay Kreps <bo...@gmail.com>.
> On Feb. 7, 2014, 4:29 a.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java, line 52
> > <https://reviews.apache.org/r/17836/diff/1/?file=479011#file479011line52>
> >
> > Does it make sense to catch all Throwables so we get meaningful error message in all cases?
>
> Jay Kreps wrote:
> No, catching Throwable is a shifty thing to do. Throwable is the union of application errors (Exception) and unrecoverable errors (OOM etc). Catching the later and calling it a SchemaException would not be good.
>
> Neha Narkhede wrote:
> Hmm.. how about catching the rest of the unrecoverable errors and at least logging a meaningful error message for those, before rethrowing those as is (which is what will happen anyways if the code hits an unrecoverable error). I just think that hitting these errors due to bugs or whatever and not having a clue of what happened is kind of annoying.
I think that is not a good idea:
http://stackoverflow.com/questions/581878/why-catch-exceptions-in-java-when-you-can-catch-throwables
We catch the Exceptions and wrap them for a good error, this should cover virtually all application bugs etc (which might actually be too much, e.g. do we really want to catch NullPointerException). The only thing that catching Throwable does is add to this java.lang.Errors. From the javadoc on Errors:
"An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch."
> On Feb. 7, 2014, 4:29 a.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java, line 53
> > <https://reviews.apache.org/r/17836/diff/1/?file=479011#file479011line53>
> >
> > This will state the field correctly. One other outstanding issue is stating the request type correctly. Would this fix that as well?
>
> Jay Kreps wrote:
> No, as I described in the JIRA the serialization code is just about reading and writing defined serialized types it doesn't and definitely shouldn't know that there is such a thing as requests.
>
> Neha Narkhede wrote:
> Understand that we don't have all the fixes now. I guess what I was saying is that it is very useful to have better error reporting in a new code base in order to understand bugs. How large is the fix to get the serde to report the right request type?
I think I'm not being clear. The serialization layer is like Avro. It doesn't log. It definitely doesn't log about kafka requests! It doesn't know about kafka requests for the same reason Avro doesn't know about kafka requests! It just knows about serialization--i.e. turning a schema and object into bytes and vice versa.
We can definitely improve logging in the Sender when we add that but please please not in the serialization library!
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17836/#review33898
-----------------------------------------------------------
On Feb. 7, 2014, 4:23 a.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17836/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2014, 4:23 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1241
> https://issues.apache.org/jira/browse/KAFKA-1241
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1241 Better error message for underflow on read of struct.
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java 0c11a04e121904b32673b0a86739e02f03066483
> clients/src/main/java/org/apache/kafka/common/protocol/types/Type.java 24ac060a82767f913dc926ccfe99d83a596e8cf3
>
> Diff: https://reviews.apache.org/r/17836/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 17836: Better error message for underflow during struct
deserialization
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17836/#review33898
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java
<https://reviews.apache.org/r/17836/#comment63613>
Does it make sense to catch all Throwables so we get meaningful error message in all cases?
clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java
<https://reviews.apache.org/r/17836/#comment63614>
This will state the field correctly. One other outstanding issue is stating the request type correctly. Would this fix that as well?
- Neha Narkhede
On Feb. 7, 2014, 4:23 a.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17836/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2014, 4:23 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1241
> https://issues.apache.org/jira/browse/KAFKA-1241
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1241 Better error message for underflow on read of struct.
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java 0c11a04e121904b32673b0a86739e02f03066483
> clients/src/main/java/org/apache/kafka/common/protocol/types/Type.java 24ac060a82767f913dc926ccfe99d83a596e8cf3
>
> Diff: https://reviews.apache.org/r/17836/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>