KAFKA-3417: Wrap reporter calls in try/catch blocks #3635
Conversation
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
There was a problem hiding this comment.
@mimaison @edoardocomar I think we need to handle client-ids consistently for metrics and quotas and I have raised KAFKA-5735 to do this in the broker and Java clients. It requires a KIP. Would you be interested in doing the KIP and implementation?
Apart from quoting metrics (which is not backward compatible), the other changes in this PR are worth doing anyway.
| @@ -133,7 +133,7 @@ static String getMBeanName(String prefix, MetricName metricName) { | |||
| mBeanName.append(","); | |||
| mBeanName.append(entry.getKey()); | |||
| mBeanName.append("="); | |||
| mBeanName.append(entry.getValue()); | |||
| mBeanName.append(ObjectName.quote(entry.getValue())); | |||
There was a problem hiding this comment.
This is an externally visible change since client-id appears quoted in metrics and can break users who rely on the existing un-quoted value.
|
Thanks for the feedback. I agree, quoting everything is a bit too invasive ! That was made on the spot WRT the quota issue. The changes you suggested in the JIRA sound good. I'm happy to work on the KIP |
|
Thanks @mimaison, I have assigned KAFKA-5735 to you. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
mimaison
changed the title
|
@rajinisivaram Do you have any objections against these changes ? Even though we've fixed the client-ids issue, any exceptions thrown by reporters could also cause quotas issues. |
|
@mimaison Perhaps we want to propagate the exception for clients? |
|
@rajinisivaram Sorry I got dragged into other things and I had forgotten about this PR ! I'm not sure propagating the exception is necessarily what we want. We wouldn't want to prevent a client running because one (badly behaving) reporter is throwing exceptions. |
Prevent exception thrown by metric reporters to impact request processing and other reporters. Co-authored-by: Mickael Maison <mickael.maison@gmail.com> Co-authored-by: Edoardo Comar <ecomar@uk.ibm.com>
|
Currently if a reporter throws an exception while a request is being processed, the broker won't even send a response back to the client. Also if there are other reporters they won't be called at all. This change prevent exceptions thrown by metric reporters to impact request processing and other reporters. |
Prevent exception thrown by metric reporters to impact request processing and other reporters. Co-authored-by: Mickael Maison <mickael.maison@gmail.com> Co-authored-by: Edoardo Comar <ecomar@uk.ibm.com> Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>
Prevent exception thrown by metric reporters to impact request processing and other reporters. Co-authored-by: Mickael Maison <mickael.maison@gmail.com> Co-authored-by: Edoardo Comar <ecomar@uk.ibm.com> Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>
No description provided.