storm
storm copied to clipboard
apache
STORM-3186: Customizable configuration for metric reporting interval
This looks fine, except I'm not clear why this is only modified for ConsolePreparableReporter.
This looks fine, except I'm not clear why this is only modified for ConsolePreparableReporter.
@agresch I have modified it for CsvPreparableReporter.java as well. These are the only two classes which had hardcoded reporting for 10 seconds.
Thanks. I also see ScheduledStormReporter using REPORT_PERIOD and REPORT_PERIOD_UNITS for setting this. Just wondering if we should switch that as well for consistency? I like the clarity of this change better. @srdo have any thoughts on this?
I think the ScheduledStormReporter classes are for the metrics reporters for the workers (the parts topologies can hook into), while this is for the daemons. I think having separate parameters makes sense, as the worker metrics config is topology level, while this new setting is at the cluster level.
Don't have an opinion on whether we should add a parameter for setting the time unit as well, do you think it would be useful?
As an aside, we really should rename StormMetricsRegistry. We have StormMetricRegistry for the workers, and StormMetricsRegistry for the daemons. It's easy to mix them up.
I think the ScheduledStormReporter classes are for the metrics reporters for the workers (the parts topologies can hook into), while this is for the daemons. I think having separate parameters makes sense, as the worker metrics config is topology level, while this new setting is at the cluster level.
Don't have an opinion on whether we should add a parameter for setting the time unit as well, do you think it would be useful?
As an aside, we really should rename StormMetricsRegistry. We have StormMetricRegistry for the workers, and StormMetricsRegistry for the daemons. It's easy to mix them up.
@srdo Let me know if its required to have a configuration for time unit as well. I would like to work on it.
@jainrish If you think a configuration for time unit would be useful, feel free to add it. Up to you whether to do it in this PR or in a later issue.
@jainrish If you think a configuration for time unit would be useful, feel free to add it. Up to you whether to do it in this PR or in a later issue.
@srdo I will prefer to do it in a separate issue. Let me know if I should raise the jira.
Looks good, just a few minor nits left:
Please add the default setting to https://github.com/apache/storm/blob/master/conf/defaults.yaml. That way it's visible to users what the default is. Then you can remove the default value from the Java classes.