[GitHub] [storm] agresch commented on a change in pull request #3318: [STORM-3670] Separate configurations for daemon metric reporters and topology metrics reporters

[GitBox <gi...@apache.org>]

agresch commented on a change in pull request #3318:
URL: https://github.com/apache/storm/pull/3318#discussion_r465113862



##########
File path: storm-server/src/main/java/org/apache/storm/daemon/metrics/MetricsUtils.java
##########
@@ -58,10 +61,11 @@ private static PreparableReporter getPreparableReporter(String clazz) {
         return reporter;
     }
 
-    public static File getCsvLogDir(Map<String, Object> topoConf) {
-        String csvMetricsLogDirectory = ObjectReader.getString(topoConf.get(DaemonConfig.STORM_DAEMON_METRICS_REPORTER_CSV_LOG_DIR), null);
+    public static File getCsvLogDir(Map<String, Object> daemonConf) {

Review comment:
       Unrelated to this PR, but it seems odd this is a metrics util thing rather than belonging to the CSV reporter class.

##########
File path: docs/metrics_v2.md
##########
@@ -72,7 +72,7 @@ public class TupleCountingBolt extends BaseRichBolt {
  For metrics to be useful they must be *reported*, in other words sent somewhere where they can be consumed and analyzed.
  That can be as simple as writing them to a log file, sending them to a time series database, or exposing them via JMX.
  
- As of Storm 1.2.0 the following metric reporters are supported
+Since Storm 1.2.0, the following metric reporters are supported

Review comment:
       I don't see the value in adding the version, I would just remove it. 

##########
File path: storm-client/src/jvm/org/apache/storm/daemon/metrics/ClientMetricsUtils.java
##########
@@ -20,26 +20,31 @@
 
 public class ClientMetricsUtils {
 
-    public static TimeUnit getMetricsRateUnit(Map<String, Object> topoConf) {
-        return getTimeUnitForCofig(topoConf, Config.STORM_DAEMON_METRICS_REPORTER_PLUGIN_RATE_UNIT);
+    private static final String RATE_UNIT = "rate.unit";
+    private static final String DURATION_UNIT = "duration.unit";
+    // Use the specified IETF BCP 47 language tag string for a Locale
+    private static final String LOCALE = "locale";
+
+    public static TimeUnit getMetricsRateUnit(Map<String, Object> reporterConf) {
+        return getTimeUnitForConfig(reporterConf, RATE_UNIT);
     }
 
-    public static TimeUnit getMetricsDurationUnit(Map<String, Object> topoConf) {
-        return getTimeUnitForCofig(topoConf, Config.STORM_DAEMON_METRICS_REPORTER_PLUGIN_DURATION_UNIT);
+    public static TimeUnit getMetricsDurationUnit(Map<String, Object> reporterConf) {
+        return getTimeUnitForConfig(reporterConf, DURATION_UNIT);
     }
 
-    public static Locale getMetricsReporterLocale(Map<String, Object> topoConf) {
-        String languageTag = ObjectReader.getString(topoConf.get(Config.STORM_DAEMON_METRICS_REPORTER_PLUGIN_LOCALE), null);
+    public static Locale getMetricsReporterLocale(Map<String, Object> reporterConf) {
+        String languageTag = ObjectReader.getString(reporterConf.get(LOCALE), null);
         if (languageTag != null) {
             return Locale.forLanguageTag(languageTag);
         }
         return null;
     }
 
-    private static TimeUnit getTimeUnitForCofig(Map<String, Object> topoConf, String configName) {
-        String rateUnitString = ObjectReader.getString(topoConf.get(configName), null);
-        if (rateUnitString != null) {
-            return TimeUnit.valueOf(rateUnitString);
+    public static TimeUnit getTimeUnitForConfig(Map<String, Object> conf, String configName) {

Review comment:
       Can you add a javadoc indicating why this is a conf vs the others being reporterConf?  Is this a backwards compatibility thing?  

##########
File path: storm-client/src/jvm/org/apache/storm/Config.java
##########
@@ -1731,21 +1744,28 @@
     @IsInteger
     @IsPositiveNumber
     public static final String WORKER_BLOB_UPDATE_POLL_INTERVAL_SECS = "worker.blob.update.poll.interval.secs";
+
     /**
-     * A specify Locale for daemon metrics reporter plugin. Use the specified IETF BCP 47 language tag string for a Locale.
+     * Specify the Locale for daemon metrics reporter plugin. Use the specified IETF BCP 47 language tag string for a Locale.
+     * This config should have been placed in the DaemonConfig class. Keeping it here only for backwards compatibility.
      */
     @IsString
     public static final String STORM_DAEMON_METRICS_REPORTER_PLUGIN_LOCALE = "storm.daemon.metrics.reporter.plugin.locale";
+
     /**
-     * A specify rate-unit in TimeUnit to specify reporting frequency for daemon metrics reporter plugin.
+     * Specify the rate unit in TimeUnit for daemon metrics reporter plugin.
+     * This config should have been placed in the DaemonConfig class. Keeping it here only for backwards compatibility.
      */
     @IsString
     public static final String STORM_DAEMON_METRICS_REPORTER_PLUGIN_RATE_UNIT = "storm.daemon.metrics.reporter.plugin.rate.unit";
+
     /**
-     * A specify duration-unit in TimeUnit to specify reporting window for daemon metrics reporter plugin.
+     * Specify the duration unit in TimeUnit for daemon metrics reporter plugin.
+     * This config should have been placed in the DaemonConfig class. Keeping it here only for backwards compatibility.

Review comment:
       Should these be deprecated if only for backwards compatibility?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org