Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added RdKafkaException base class #231

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

accelerated
Copy link
Contributor

  • Added RdKafkaException base class for exceptions which take an Error as parameter. This allows for easier catching and processing of kafka-specific errors.
  • In cases where the error message was sinked into the Exception (ParseException and ActionTerminatedException), the signature is pass-by-copy now to be consistent with the base Exception class. This should not break ABI since the constructor is only called internally.

@accelerated
Copy link
Contributor Author

Hi @mfontanini, can you please take a look at this? ~thanks

@mfontanini
Copy link
Owner

mfontanini commented Jan 13, 2020

While I'm okay with these changes and this should probably have been done this way from the start, this change breaks the ABI and API. This:

  • Assumes nobody is throwing these exceptions manually (e.g. while testing code).
  • Breaks the API as code calling e.g. QueueException::get_error will now fail to link.
  • (potentially) changes the layout of these exceptions. I think in practice they'll end up having the same layout but it's a risky change.

Having said that, I don't think it's worth bumping the major version for such a simple change.

@accelerated
Copy link
Contributor Author

Agreed with you. I think it's a worthwhile change with small breakage risk.

@accelerated
Copy link
Contributor Author

@mfontanini are you ok going forward with this? Can we merge it? Otherwise I'll close it.

@accelerated
Copy link
Contributor Author

@mfontanini please check my last comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants