-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
KAFKA-5066: Add KafkaMetricsConfig (Yammer metrics reporters) props to documentation #5563
Conversation
@hachikuji Please take a look at this minor change for doc purpose. |
@lindong28 Please take a look whenever you get a chance. This is a minor change for doc purpose. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch @omkreddy. Looks good overall. Left only one minor comment.
@@ -22,16 +22,33 @@ package kafka.metrics | |||
|
|||
import kafka.utils.{VerifiableProperties, CoreUtils} | |||
|
|||
object KafkaMetricsConfig { | |||
|
|||
val KafkaMetricsReporterClassesProp = "kafka.metrics.reporters" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be more readable and consistent with existing code style by moving these variables (property name, doc and default value) to KafkaConfig.java?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lindong28 Makes sense. Updated the PR to make it consistent with existing configs.
8538076
to
c2e5961
Compare
c2e5961
to
07e9fe0
Compare
|
||
class KafkaMetricsConfig(props: VerifiableProperties) { | ||
|
||
/** | ||
* Comma-separated list of reporter types. These classes should be on the | ||
* classpath and will be instantiated at run-time. | ||
*/ | ||
val reporters = CoreUtils.parseCsvList(props.getString("kafka.metrics.reporters", "")) | ||
val reporters = CoreUtils.parseCsvList(props.getString(KafkaConfig.KafkaMetricsReporterClassesProp, "")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. Just one more minor comment. Would it be slightly better to also define KafkaMetricsReporterClasses = ""
in kafka.serverDefaults
, similar to the existing Defaults.MetricReporterClasses = ""
? It seems that in the existing KafkaConfig.java, we always use use a default variable with value ""
rather than using ""
directly. Otherwise LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lindong28 Thanks for the review. Updated the PR.
Thanks for the patch @omkreddy :) |
…o documentation Author: Manikumar Reddy <manikumar.reddy@gmail.com> Reviewers: Dong Lin <lindong28@gmail.com> Closes apache#5563 from omkreddy/KAFKA-5066-KAFKA-METRICS-CONFIG
Committer Checklist (excluded from commit message)