confluentinc / confluent-kafka-dotnet

Confluent's Apache Kafka .NET client
https://github.com/confluentinc/confluent-kafka-dotnet/wiki
Apache License 2.0
59 stars 861 forks source link

Change some Exception to KafkaException #312

Open treziac opened 7 years ago

treziac commented 7 years ago

In SafeKafkaHandle / SafeConfigHandle, we throw Exception in several case when librdkafka return an error (list cannot be created, configuration not valid, etc). Throwing base exception is discouraged (https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/exceptions/creating-and-throwing-exceptions#things-to-avoid-when-throwing-exceptions) Should we not rather throw KafkaException (with ErrorCode.LocalFail or ErrorCode.LocalInvalidArg given the case), to detect that it comes from librdkafka? (is is always preferred to throw a typed exception) We should still throw InvalidException or other when the exception is thrown before any call to librdkafka (argument check...)

This is a possible breaking change (for people checking the type of the exception), but I think it's worth it

mhowlett commented 7 years ago

Looking at the rd_topic_partition_list_new function in librdkafka, it seems as though it can never return IntPtr.Zero because after allocation, the method makes use of the pointer, and no check for NULL is made, so the unmanaged code will blow up first. We could just remove these checks, or otherwise I agree with your idea that KafkaException is better here (and prefer ErrorCode.LocalFail). Will accept a PR for either :-).