KAFKA-3318: clean up consumer logging and error messages#1036
KAFKA-3318: clean up consumer logging and error messages#1036hachikuji wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Typo "poll() by with max.poll.records"?
|
Minor comment, otherwise LGTM! |
There was a problem hiding this comment.
Do we really need two log statements so near to each other? I guess the second one also includes the co-ordinator id. Would it be too confusing to include it in the previous log statement?
There was a problem hiding this comment.
The coordinator id ends up being a really big negative integer, but if we used host:port in the log message then maybe it wouldn't be too bad? We'd probably want to make the same change in the other messages as well.
There was a problem hiding this comment.
Interesting. Which is more useful host:port or id? If the latter, let's just leave as is. Otherwise, then changing it here and other places sounds good.
There was a problem hiding this comment.
Usually the host:port is more helpful when debugging, and it's kind of a pain to do the reverse mapping from id to host in my head. The only time the id is helpful is when looking at NetworkClient issues.
|
Thanks for this @hachikuji, very important. :) I left a few comments, looks good otherwise. |
|
@ijuma Made some more improvements. In particular, I wanted to ensure that the groupId was always provided since otherwise it's difficult to follow logs which contain multiple consumer instances. I also tried to address the review comments. |
There was a problem hiding this comment.
Should we change the toString of Node to be more logging friendly?
There was a problem hiding this comment.
It currently looks like "Node(id, host, port)," which doesn't seem too bad to me. What do you think?
There was a problem hiding this comment.
When I suggested it, I thought we could do a bit better, maybe something like (id: 5, www.example.com:9123), but maybe that's actually worse.
There was a problem hiding this comment.
I was mostly trying to get rid of the word Node because it's a bit redundant when you look at the log messages.
There was a problem hiding this comment.
Initially I didn't use Node's toString() at all. I just filled in "{}:{}" with the host and port, but that got a little annoying after a couple times. But maybe it's actually better?
|
@hachikuji I did another pass. That last commit changed more than the previous ones! I think it's looking pretty good now. I had a small number of comments/questions. |
…ormation available
|
LGTM |
4 participants
No description provided.