flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [flink] rionmonster commented on a change in pull request #15165: [FLINK-16829] Refactored Prometheus Metric Reporters to Use Constructor Initialization
Date Fri, 12 Mar 2021 15:58:47 GMT

rionmonster commented on a change in pull request #15165:
URL: https://github.com/apache/flink/pull/15165#discussion_r593279557



##########
File path: flink-metrics/flink-metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/PrometheusPushGatewayReporter.java
##########
@@ -56,43 +56,42 @@
     private boolean deleteOnShutdown;
     private Map<String, String> groupingKey;
 
-    @Override
-    public void open(MetricConfig config) {
-        super.open(config);
-
-        String host = config.getString(HOST.key(), HOST.defaultValue());
-        int port = config.getInteger(PORT.key(), PORT.defaultValue());
-        String configuredJobName = config.getString(JOB_NAME.key(), JOB_NAME.defaultValue());
-        boolean randomSuffix =
-                config.getBoolean(
-                        RANDOM_JOB_NAME_SUFFIX.key(), RANDOM_JOB_NAME_SUFFIX.defaultValue());
-        deleteOnShutdown =
-                config.getBoolean(DELETE_ON_SHUTDOWN.key(), DELETE_ON_SHUTDOWN.defaultValue());
-        groupingKey =
-                parseGroupingKey(config.getString(GROUPING_KEY.key(), GROUPING_KEY.defaultValue()));
-
-        if (host == null || host.isEmpty() || port < 1) {
+    PrometheusPushGatewayReporter(
+        @Nullable final String hostConfig,
+        @Nullable final int portConfig,
+        @Nullable final String jobNameConfig,
+        @Nullable final boolean randomJobSuffixConfig,
+        @Nullable final boolean deleteOnShutdownConfig,
+        @Nullable final Map<String, String> groupingKeyConfig
+    ) {
+        deleteOnShutdown = deleteOnShutdownConfig
+        groupingKey = parseGroupingKey(groupingKeyConfig)
+
+        if (hostConfig == null || hostConfig.isEmpty() || portConfig < 1) {
             throw new IllegalArgumentException(
-                    "Invalid host/port configuration. Host: " + host + " Port: " + port);
+                    "Invalid host/port configuration. Host: " + hostConfig + " Port: " +
portConfig);
         }
 
-        if (randomSuffix) {
-            this.jobName = configuredJobName + new AbstractID();
+        if (randomJobSuffixConfig) {
+            this.jobName = jobNameConfig + new AbstractID();
         } else {
-            this.jobName = configuredJobName;
+            this.jobName = jobNameConfig;
         }
 
-        pushGateway = new PushGateway(host + ':' + port);
+        pushGateway = new PushGateway(hostConfig + ':' + portConfig);
         log.info(
                 "Configured PrometheusPushGatewayReporter with {host:{}, port:{}, jobName:{},
randomJobNameSuffix:{}, deleteOnShutdown:{}, groupingKey:{}}",
-                host,
-                port,
-                jobName,
-                randomSuffix,
+                hostConfig,
+                portConfig,
+                jobNameConfig,
+                randomJobSuffixConfig,
                 deleteOnShutdown,
                 groupingKey);
     }
 
+    @Override
+    public void open(MetricConfig config) { }
+
     Map<String, String> parseGroupingKey(final String groupingKeyConfig) {

Review comment:
       Thanks for the feedback!
   
   This makes sense from a testability perspective. Since the factory only presently has a
`createMetricReporter` method defined on the interface, I'm guessing this static method would
just support the sanitization / cleanup calls prior to just calling the other method?
   
   Something like:
   
   ```
   static PrometheusPushGatewayReporter nameTbd(Properties properties)
       {
           // Check for nulls, general sanitization code here
   
           return createMetricReporter(...);
       }
   
       @Override
       public PrometheusPushGatewayReporter createMetricReporter(Properties properties) {
           // Omitted for brevity 
           return new PrometheusPushGatewayReporter(...);
       }
   ```
   
   Because otherwise, it just seems like `open` (or whatever it ends up being named) would
just circumvent the need for a factory unless I'm just misunderstanding what the static method
would do.




----------------------------------------------------------------
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



Mime
View raw message