[GitHub] storm pull request #2554: STORM-2939 add WorkerMetricsProcessor interface
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
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
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
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
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
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 ---