Skip to content
This repository has been archived by the owner on Mar 24, 2021. It is now read-only.

Please do not use Broker-side exceptions for Client-side errors #697

Closed
jeffwidman opened this issue Jul 1, 2017 · 3 comments · Fixed by #713
Closed

Please do not use Broker-side exceptions for Client-side errors #697

jeffwidman opened this issue Jul 1, 2017 · 3 comments · Fixed by #713
Assignees
@emmettbutler
Labels
bug

Comments

@jeffwidman
Copy link
Contributor

jeffwidman commented Jul 1, 2017 •
edited
Loading

When debugging #696 where the broker successfully returns a message to the consumer and then the consumer blows up, I was confused because the error that was raised was MessageSizeTooLarge which is a broker error code number 10: https://kafka.apache.org/protocol.html#protocol_error_codes

This was extremely confusing to me because initial spelunking in the pykafka source showed that this exception subclassed ProtocolClientError, indicating that the broker was returning the error code:

pykafka/pykafka/exceptions.py

Line 138 in c73f060

class MessageSizeTooLarge(ProtocolClientError):

Furthermore, it made no sense contextually because that error code is only emitted by the broker to a producer trying to produce too large of a message, not to a consumer.

It had me scratching my head until I realized that pykafka was re-using what is semantically a broker-side exception for a client-side error: https://github.com/Parsely/pykafka/blame/c80f66c0d0b11d830aa333ed486967b5f242bc2f/pykafka/protocol.py#L383

Digging a little deeper, I see that originally this exception wasn't the same as a broker-side error, but in #500 @emmett9001 changed it to re-use that exception class based on discussion in #497.

Can I request that you limit the use of broker-side exceptions to only situations where the broker is actually returning an error code?

In this particular case, the proper fix is probably removing this particular usage of the exception altogether as suggested in #696. If you choose to keep it a hard-limit and throw an exception, can you use an exception that more clearly indicates a client-side error? Probably with a suggestion to the user to increase their fetch_message_max_bytes setting.
@amontalenti
Copy link
Contributor

@jeffwidman Thanks for reporting this. @emmett9001 will look into this when he gets a chance.

@emmettbutler
Copy link
Contributor

Thanks @jeffwidman, this is a reasonable request that I think is worth auditing the codebase for.

@emmettbutler emmettbutler mentioned this issue Jul 6, 2017
Open
emmettbutler added a commit that referenced this issue Aug 24, 2017
@emmettbutler
fix exception when messageset is too large for decode buffer
92b75f3 
new exception does not inherit from ProtocolClientError, removing ambiguity about whether this error originated at the broker or the client

fixes #697
This was referenced Aug 24, 2017
Merged
Merged
@jeffwidman
Copy link
Contributor Author

jeffwidman commented Aug 28, 2017 •
edited
Loading

Thanks!

And for doing the work to audit to codebase.

@jeffwidman jeffwidman mentioned this issue Oct 3, 2017
Closed
@jeffwidman jeffwidman mentioned this issue Nov 8, 2018
Closed
jeffwidman added a commit to dpkp/kafka-python that referenced this issue Nov 17, 2018
@jeffwidman
Stop using broker-errors for client-side problems
6c11911 
`UnsupportedVersionError` is intended to indicate a server-side error:
https://github.com/dpkp/kafka-python/blob/ba7372e44ffa1ee49fb4d5efbd67534393e944db/kafka/errors.py#L375-L378

So we should not be raising it for client-side errors. I realize that
semantically this seems like the appropriate error to raise. However,
this is confusing when debugging... for a real-life example, see
Parsely/pykafka#697. So I strongly feel that
server-side errors should be kept separate from client-side errors,
even if all the client is doing is proactively protecting against
hitting a situation where the broker would return this error.
@jeffwidman jeffwidman mentioned this issue Nov 17, 2018
Merged
jeffwidman added a commit to dpkp/kafka-python that referenced this issue Nov 17, 2018
@jeffwidman
Stop using broker-errors for client-side problems
7da8eb1 
`UnsupportedVersionError` is intended to indicate a server-side error:
https://github.com/dpkp/kafka-python/blob/ba7372e44ffa1ee49fb4d5efbd67534393e944db/kafka/errors.py#L375-L378

So we should not be raising it for client-side errors. I realize that
semantically this seems like the appropriate error to raise. However,
this is confusing when debugging... for a real-life example, see
Parsely/pykafka#697. So I strongly feel that
server-side errors should be kept separate from client-side errors,
even if all the client is doing is proactively protecting against
hitting a situation where the broker would return this error.
jeffwidman added a commit to dpkp/kafka-python that referenced this issue Nov 17, 2018
@jeffwidman
Stop using broker-errors for client-side problems
dd8d304 
`UnsupportedVersionError` is intended to indicate a server-side error:
https://github.com/dpkp/kafka-python/blob/ba7372e44ffa1ee49fb4d5efbd67534393e944db/kafka/errors.py#L375-L378

So we should not be raising it for client-side errors. I realize that
semantically this seems like the appropriate error to raise. However,
this is confusing when debugging... for a real-life example, see
Parsely/pykafka#697. So I strongly feel that
server-side errors should be kept separate from client-side errors,
even if all the client is doing is proactively protecting against
hitting a situation where the broker would return this error.
jeffwidman added a commit to dpkp/kafka-python that referenced this issue Nov 18, 2018
@jeffwidman
Stop using broker-errors for client-side problems
aad278f 
`UnsupportedVersionError` is intended to indicate a server-side error:
https://github.com/dpkp/kafka-python/blob/ba7372e44ffa1ee49fb4d5efbd67534393e944db/kafka/errors.py#L375-L378

So we should not be raising it for client-side errors. I realize that
semantically this seems like the appropriate error to raise. However,
this is confusing when debugging... for a real-life example, see
Parsely/pykafka#697. So I strongly feel that
server-side errors should be kept separate from client-side errors,
even if all the client is doing is proactively protecting against
hitting a situation where the broker would return this error.
jeffwidman added a commit to dpkp/kafka-python that referenced this issue Nov 18, 2018
@jeffwidman
Stop using broker-errors for client-side problems
60a100c 
`UnsupportedVersionError` is intended to indicate a server-side error:
https://github.com/dpkp/kafka-python/blob/ba7372e44ffa1ee49fb4d5efbd67534393e944db/kafka/errors.py#L375-L378

So we should not be raising it for client-side errors. I realize that
semantically this seems like the appropriate error to raise. However,
this is confusing when debugging... for a real-life example, see
Parsely/pykafka#697. So I strongly feel that
server-side errors should be kept separate from client-side errors,
even if all the client is doing is proactively protecting against
hitting a situation where the broker would return this error.
jeffwidman added a commit to dpkp/kafka-python that referenced this issue Nov 18, 2018
@jeffwidman
Stop using broker-errors for client-side problems
c861fec 
`UnsupportedVersionError` is intended to indicate a server-side error:
https://github.com/dpkp/kafka-python/blob/ba7372e44ffa1ee49fb4d5efbd67534393e944db/kafka/errors.py#L375-L378

So we should not be raising it for client-side errors. I realize that
semantically this seems like the appropriate error to raise. However,
this is confusing when debugging... for a real-life example, see
Parsely/pykafka#697. So I strongly feel that
server-side errors should be kept separate from client-side errors,
even if all the client is doing is proactively protecting against
hitting a situation where the broker would return this error.
jeffwidman added a commit to dpkp/kafka-python that referenced this issue Nov 18, 2018
@jeffwidman
Stop using broker-errors for client-side problems
c98f136 
`UnsupportedVersionError` is intended to indicate a server-side error:
https://github.com/dpkp/kafka-python/blob/ba7372e44ffa1ee49fb4d5efbd67534393e944db/kafka/errors.py#L375-L378

So we should not be raising it for client-side errors. I realize that
semantically this seems like the appropriate error to raise. However,
this is confusing when debugging... for a real-life example, see
Parsely/pykafka#697. So I strongly feel that
server-side errors should be kept separate from client-side errors,
even if all the client is doing is proactively protecting against
hitting a situation where the broker would return this error.
jeffwidman added a commit to dpkp/kafka-python that referenced this issue Nov 18, 2018
@jeffwidman
Stop using broker-errors for client-side problems
d5d498e 
`UnsupportedVersionError` is intended to indicate a server-side error:
https://github.com/dpkp/kafka-python/blob/ba7372e44ffa1ee49fb4d5efbd67534393e944db/kafka/errors.py#L375-L378

So we should not be raising it for client-side errors. I realize that
semantically this seems like the appropriate error to raise. However,
this is confusing when debugging... for a real-life example, see
Parsely/pykafka#697. So I strongly feel that
server-side errors should be kept separate from client-side errors,
even if all the client is doing is proactively protecting against
hitting a situation where the broker would return this error.
jeffwidman added a commit to dpkp/kafka-python that referenced this issue Nov 18, 2018
@jeffwidman
Stop using broker-errors for client-side problems
96da529 
`UnsupportedVersionError` is intended to indicate a server-side error:
https://github.com/dpkp/kafka-python/blob/ba7372e44ffa1ee49fb4d5efbd67534393e944db/kafka/errors.py#L375-L378

So we should not be raising it for client-side errors. I realize that
semantically this seems like the appropriate error to raise. However,
this is confusing when debugging... for a real-life example, see
Parsely/pykafka#697. So I strongly feel that
server-side errors should be kept separate from client-side errors,
even if all the client is doing is proactively protecting against
hitting a situation where the broker would return this error.
jeffwidman added a commit to dpkp/kafka-python that referenced this issue Nov 18, 2018
@jeffwidman
Stop using broker-errors for client-side problems
1308efc 
`UnsupportedVersionError` is intended to indicate a server-side error:
https://github.com/dpkp/kafka-python/blob/ba7372e44ffa1ee49fb4d5efbd67534393e944db/kafka/errors.py#L375-L378

So we should not be raising it for client-side errors. I realize that
semantically this seems like the appropriate error to raise. However,
this is confusing when debugging... for a real-life example, see
Parsely/pykafka#697. So I strongly feel that
server-side errors should be kept separate from client-side errors,
even if all the client is doing is proactively protecting against
hitting a situation where the broker would return this error.
jeffwidman added a commit to dpkp/kafka-python that referenced this issue Nov 18, 2018
@jeffwidman
Stop using broker-errors for client-side problems
f3105a4 
`UnsupportedVersionError` is intended to indicate a server-side error:
https://github.com/dpkp/kafka-python/blob/ba7372e44ffa1ee49fb4d5efbd67534393e944db/kafka/errors.py#L375-L378

So we should not be raising it for client-side errors. I realize that
semantically this seems like the appropriate error to raise. However,
this is confusing when debugging... for a real-life example, see
Parsely/pykafka#697. So I strongly feel that
server-side errors should be kept separate from client-side errors,
even if all the client is doing is proactively protecting against
hitting a situation where the broker would return this error.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@emmettbutler emmettbutler

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix exception when messageset is too large for decode buffer Parsely/pykafka
3 participants
@amontalenti @jeffwidman @emmettbutler