[GitHub] storm pull request #2554: STORM-2939 add WorkerMetricsProcessor interface

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/storm/pull/2554


---

[GitHub] storm pull request #2554: STORM-2939 add WorkerMetricsProcessor interface

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2554#discussion_r167608762
  
--- Diff: 
storm-server/src/main/java/org/apache/storm/metricstore/WorkerMetricsProcessor.java
 ---
@@ -0,0 +1,40 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.storm.metricstore;
+
+import java.util.Map;
+import org.apache.storm.generated.WorkerMetrics;
+
+public interface WorkerMetricsProcessor {
+
+/**
+ * Process insertion of worker metrics.
+ * @param conf Storm config map
--- End diff --

Can we make sure that we distinguish between the topology config here, and 
the daemon config that is passed into prepare?


---

[GitHub] storm pull request #2554: STORM-2939 add WorkerMetricsProcessor interface

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2554#discussion_r167608546
  
--- Diff: 
storm-server/src/main/java/org/apache/storm/metricstore/WorkerMetricsProcessor.java
 ---
@@ -0,0 +1,40 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.storm.metricstore;
+
+import java.util.Map;
+import org.apache.storm.generated.WorkerMetrics;
+
+public interface WorkerMetricsProcessor {
+
+/**
+ * Process insertion of worker metrics.
--- End diff --

Can we mention something about thread safety in here?


---

[GitHub] storm pull request #2554: STORM-2939 add WorkerMetricsProcessor interface

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2554#discussion_r167607557
  
--- Diff: storm-server/src/main/java/org/apache/storm/DaemonConfig.java ---
@@ -1040,6 +1039,13 @@
 // and log an error message, so just validating this as a String for 
now.
 public static final String STORM_METRIC_STORE_CLASS = 
"storm.metricstore.class";
 
+/**
+ * Class implementing WorkerMetricsProcessor.
--- End diff --

It might be good to mention here in the javadocs that this is intended to 
be run on the supervisor.


---

[GitHub] storm pull request #2554: STORM-2939 add WorkerMetricsProcessor interface

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2554#discussion_r167608078
  
--- Diff: 
storm-server/src/main/java/org/apache/storm/metricstore/MetricStoreConfig.java 
---
@@ -41,5 +41,23 @@ public static MetricStore configure(Map conf) throws 
MetricException {
 throw new MetricException("Failed to create metric store", t);
 }
 }
+
+/**
+ * Configures metric processor to use the class specified in the conf.
+ * @param conf Storm config map
+ * @return WorkerMetricsProcessor prepared processor
+ * @throws MetricException  on misconfiguration
+ */
+public static WorkerMetricsProcessor configureMetricProcessor(Map 
conf) throws MetricException {
+
+try {
+String processorClass = 
(String)conf.get(DaemonConfig.STORM_METRIC_PROCESSOR_CLASS);
+WorkerMetricsProcessor processor = (WorkerMetricsProcessor) 
(Class.forName(processorClass)).newInstance();
+processor.prepare(conf);
+return processor;
+} catch (Throwable t) {
--- End diff --

Can we just make this an Exception instead of a Throwable?  Most Errors are 
not things that we want to try and recover from.


---

[GitHub] storm pull request #2554: STORM-2939 add WorkerMetricsProcessor interface

[agresch]

GitHub user agresch opened a pull request:

https://github.com/apache/storm/pull/2554

STORM-2939 add WorkerMetricsProcessor interface

This adds the WorkerMetricsProcessor interface.  The default implementation 
executes the current code that sends metric data to Nimbus for insertion, now 
done by NimbusMetricProcessor.

The intent is that we can replace this class for other metricstore 
implementations (HBase is what I am currently working on) that can instead do 
insertion directly.



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/agresch/storm agresch_workermetricprocessor

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/storm/pull/2554.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #2554


commit 1eb355ef7825740686e2294238c51f3495cbeb88
Author: Aaron Gresch 
Date:   2018-02-08T22:09:19Z

STORM-2939 add WorkerMetricsProcessor interface




---