Log error in case consumer destructor throws #61
Conversation
include/cppkafka/configuration.h
Outdated
| @@ -45,6 +45,8 @@ | |||
|
|
|||
| namespace cppkafka { | |||
|
|
|||
| constexpr const char* library_name = "cppkafka"; | |||
There was a problem hiding this comment.
Can you just move this into Consumer::~Consumer? This is kind of out of place up here.
src/consumer.cpp
Outdated
| @@ -26,7 +26,7 @@ | |||
| * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | |||
| * | |||
| */ | |||
|
|
|||
| #include <syslog.h> | |||
There was a problem hiding this comment.
This header doesn't exist on Windows. I'm not sure how rdkafka does this so it works on all platforms but this should do the same.
src/consumer.cpp
Outdated
| // If close throws just silently ignore until there's some | ||
| // logging facility (if any) | ||
| catch (const Exception& ex) { | ||
| auto& callback = get_configuration().get_log_callback(); |
There was a problem hiding this comment.
Can you make this const auto&? I prefer to explicitly indicate something is const instead of expecting the user to know that's the case.
src/consumer.cpp
Outdated
| catch (const Exception& ex) { | ||
| auto& callback = get_configuration().get_log_callback(); | ||
| if (callback) { | ||
| callback(*this, LOG_ERR, library_name, ex.what()); |
There was a problem hiding this comment.
Maybe this should actually add some extra information here? I'm not sure the exception message would be enough for the user to realize what happened. Maybe something along the lines of "Failed to close consumer: <error-message>"?
|
Ok will address all these. |
accelerated
force-pushed
the
consumer_destructor
branch
4 times, most recently
from
1b2ceb2 to
dfba856
Compare
|
i'll double check for windows compatibility next time...I keep forgetting :) |
accelerated
force-pushed
the
consumer_destructor
branch
from
dfba856 to
d55edb2
Compare
include/cppkafka/exceptions.h
Outdated
| @@ -38,6 +38,18 @@ | |||
|
|
|||
| namespace cppkafka { | |||
|
|
|||
| // Based on syslog.h levels | |||
| enum LogLevel { | |||
There was a problem hiding this comment.
Maybe this belongs on a new cppkafka/logging.h file? Not sure what else would go there but I think it's slightly off in this exceptions.h. Also, I'd use enum class so this is better scoped.
There was a problem hiding this comment.
Ok. I didn't use the class as you are not using it anywhere else, but I agree it's a better approach. Unfortunately this means that convertion enum->int has to be cast now.
There was a problem hiding this comment.
Looks like it's being used on half on the cases. This could be cleaned up in some major version.
src/consumer.cpp
Outdated
| // logging facility (if any) | ||
| catch (const Exception& ex) { | ||
| constexpr const char* library_name = "cppkafka"; | ||
| std::ostringstream error_msg; |
There was a problem hiding this comment.
Minor: use using std::ostringstream after include directives and remove the std:: specifier here.
accelerated
force-pushed
the
consumer_destructor
branch
from
d55edb2 to
0f996a7
Compare
No description provided.