[GitHub] storm pull request: STORM-1723 (1.x) Introduce ClusterMetricsConsu...
Github user abhishekagarwal87 commented on a diff in the pull request: https://github.com/apache/storm/pull/1352#discussion_r61565297 --- Diff: conf/defaults.yaml --- @@ -285,3 +285,6 @@ pacemaker.kerberos.users: [] #default storm daemon metrics reporter plugins storm.daemon.metrics.reporter.plugins: - "org.apache.storm.daemon.metrics.reporters.JmxPreparableReporter" --- End diff -- Ahh I see. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-1723 (1.x) Introduce ClusterMetricsConsu...
Github user HeartSaVioR commented on a diff in the pull request: https://github.com/apache/storm/pull/1352#discussion_r61556669 --- Diff: conf/defaults.yaml --- @@ -285,3 +285,6 @@ pacemaker.kerberos.users: [] #default storm daemon metrics reporter plugins storm.daemon.metrics.reporter.plugins: - "org.apache.storm.daemon.metrics.reporters.JmxPreparableReporter" --- End diff -- Please refer this file: https://github.com/apache/storm/blob/master/docs/storm-metrics-profiling-internal-actions.md (btw, link for http://storm.apache.org/releases/1.0.0/storm-metrics-profiling-internal-actions.html is broken, and so 2.0.0-SNAPSHOT is.) Codahale metrics is included as shaded so we don't guarantee other plugins are working properly. We want to expose those metrics to consumer to store external services/storages, and where to store should be flexible by make it pluggable. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-1723 (1.x) Introduce ClusterMetricsConsu...
Github user abhishekagarwal87 commented on a diff in the pull request: https://github.com/apache/storm/pull/1352#discussion_r61555323 --- Diff: conf/defaults.yaml --- @@ -285,3 +285,6 @@ pacemaker.kerberos.users: [] #default storm daemon metrics reporter plugins storm.daemon.metrics.reporter.plugins: - "org.apache.storm.daemon.metrics.reporters.JmxPreparableReporter" --- End diff -- can't we piggyback on codahale to report these metrics? We can have one more config ```storm.cluster.metrics.reporter.plugins: - "org.apache.storm.daemon.metrics.reporters.JmxPreparableReporter" ``` I haven't gone through diff in nimbus completely but I suppose they can be modeled as gauages. It would be lot more simplified in my opinion. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-1723 (1.x) Introduce ClusterMetricsConsu...
Github user unsleepy22 commented on the pull request: https://github.com/apache/storm/pull/1352#issuecomment-214360458 +1 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-1723 (1.x) Introduce ClusterMetricsConsu...
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/1352#issuecomment-214096856 Updating diff files again. [STORM-1723-1.x-v2-diff.txt](https://github.com/apache/storm/files/233825/STORM-1723-1.x-v2-diff.txt) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-1723 (1.x) Introduce ClusterMetricsConsu...
Github user unsleepy22 commented on the pull request: https://github.com/apache/storm/pull/1352#issuecomment-214096038 Maybe it's better to put multi-cluster support in 2.0, in which I think multi-cluster can be supported at least in web ui. So we can leave it be for now if we don't see the need from end users. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-1723 (1.x) Introduce ClusterMetricsConsu...
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/1352#issuecomment-214090708 Also addressed some missed spots. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-1723 (1.x) Introduce ClusterMetricsConsu...
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/1352#issuecomment-214085814 @unsleepy22 About 'cluster.id', I agree what you considered, but also we need to consider that Storm itself is not aware of multiple clusters. So IMO it would be not better to name cluster by Storm side. But we can open possibility to provide option to name cluster id (`storm.cluster.id`) from yaml configuration. In fact setting cluster id to UUID automatically doesn't help much since users can't recognize its id. What do you think? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-1723 (1.x) Introduce ClusterMetricsConsu...
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/1352#issuecomment-214081825 @unsleepy22 Oh there's a missing spot, ClusterConsumerExecutor should be renamed to ClusterMetricsConsumerExecutor. I would like to replace 'executor' from that name but I couldn't find better name due to short English. If we're OK to only rename 'ClusterConsumerExecutor' to 'ClusterMetricsConsumerExecutor', I think renaming 'cluster-consumer-executors' to 'cluster-metrics-consumer-executors' makes sense, too. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-1723 (1.x) Introduce ClusterMetricsConsu...
Github user unsleepy22 commented on the pull request: https://github.com/apache/storm/pull/1352#issuecomment-213391769 for the ClusterInfo class, do we need to add a "cluster.name" property? In a real metrics consumer case, the external storage may want to consumer a bunch of cluster metrics and display them, thus a "cluster.name" may be necessary to distinguish different clusters. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-1723 (1.x) Introduce ClusterMetricsConsu...
Github user unsleepy22 commented on the pull request: https://github.com/apache/storm/pull/1352#issuecomment-213390625 I couldn't comment on nimbus.clj, how about rename "cluster-consumer-executors" to "cluster-metrics-consumers" since we have "executors" in tasks already. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-1723 (1.x) Introduce ClusterMetricsConsu...
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1352#discussion_r60589092
--- Diff:
storm-core/src/jvm/org/apache/storm/metric/ClusterConsumerExecutor.java ---
@@ -0,0 +1,87 @@
+/**
+ * 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.metric;
+
+import com.google.common.util.concurrent.MoreExecutors;
+import org.apache.storm.Config;
+import org.apache.storm.metric.api.DataPoint;
+import org.apache.storm.metric.api.IClusterMetricsConsumer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+public class ClusterConsumerExecutor {
+public static final Logger LOG =
LoggerFactory.getLogger(ClusterConsumerExecutor.class);
+
+private IClusterMetricsConsumer metricsConsumer;
+private String consumerClassName;
+private Object registerationArgument;
+private ExecutorService consumerExecutorService;
+
+public ClusterConsumerExecutor(String consumerClassName, Object
registerationArgument) {
+this.consumerClassName = consumerClassName;
+this.registerationArgument = registerationArgument;
--- End diff --
Thanks, addressed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1723 (1.x) Introduce ClusterMetricsConsu...
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1352#discussion_r60585330
--- Diff:
storm-core/src/jvm/org/apache/storm/metric/ClusterConsumerExecutor.java ---
@@ -0,0 +1,87 @@
+/**
+ * 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.metric;
+
+import com.google.common.util.concurrent.MoreExecutors;
+import org.apache.storm.Config;
+import org.apache.storm.metric.api.DataPoint;
+import org.apache.storm.metric.api.IClusterMetricsConsumer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+public class ClusterConsumerExecutor {
+public static final Logger LOG =
LoggerFactory.getLogger(ClusterConsumerExecutor.class);
+
+private IClusterMetricsConsumer metricsConsumer;
+private String consumerClassName;
+private Object registerationArgument;
+private ExecutorService consumerExecutorService;
+
+public ClusterConsumerExecutor(String consumerClassName, Object
registerationArgument) {
+this.consumerClassName = consumerClassName;
+this.registerationArgument = registerationArgument;
+}
+
+public void prepare() {
+try {
+metricsConsumer =
(IClusterMetricsConsumer)Class.forName(consumerClassName).newInstance();
--- End diff --
Yes right.
Users are encouraged to package their consumer as a fat jar and place to
system environment STORM_EXT_CLASSPATH_DAEMON or /extlib-daemon.
Placing jar to system environment STORM_EXT_CLASSPATH or /extlib also works
but it will affect non-daemon (including worker) so former would be better way.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1723 (1.x) Introduce ClusterMetricsConsu...
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1352#discussion_r60571218
--- Diff:
storm-core/src/jvm/org/apache/storm/metric/ClusterConsumerExecutor.java ---
@@ -0,0 +1,87 @@
+/**
+ * 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.metric;
+
+import com.google.common.util.concurrent.MoreExecutors;
+import org.apache.storm.Config;
+import org.apache.storm.metric.api.DataPoint;
+import org.apache.storm.metric.api.IClusterMetricsConsumer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+public class ClusterConsumerExecutor {
+public static final Logger LOG =
LoggerFactory.getLogger(ClusterConsumerExecutor.class);
+
+private IClusterMetricsConsumer metricsConsumer;
+private String consumerClassName;
+private Object registerationArgument;
+private ExecutorService consumerExecutorService;
+
+public ClusterConsumerExecutor(String consumerClassName, Object
registerationArgument) {
+this.consumerClassName = consumerClassName;
+this.registerationArgument = registerationArgument;
--- End diff --
Nice finding. I'll address it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1723 (1.x) Introduce ClusterMetricsConsu...
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1352#discussion_r60570517
--- Diff:
storm-core/src/jvm/org/apache/storm/metric/ClusterConsumerExecutor.java ---
@@ -0,0 +1,87 @@
+/**
+ * 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.metric;
+
+import com.google.common.util.concurrent.MoreExecutors;
+import org.apache.storm.Config;
+import org.apache.storm.metric.api.DataPoint;
+import org.apache.storm.metric.api.IClusterMetricsConsumer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+public class ClusterConsumerExecutor {
+public static final Logger LOG =
LoggerFactory.getLogger(ClusterConsumerExecutor.class);
+
+private IClusterMetricsConsumer metricsConsumer;
+private String consumerClassName;
+private Object registerationArgument;
+private ExecutorService consumerExecutorService;
+
+public ClusterConsumerExecutor(String consumerClassName, Object
registerationArgument) {
+this.consumerClassName = consumerClassName;
+this.registerationArgument = registerationArgument;
+}
+
+public void prepare() {
+try {
+metricsConsumer =
(IClusterMetricsConsumer)Class.forName(consumerClassName).newInstance();
--- End diff --
Registered `consumerClassName` should be available as part of the same
classloader as `ClusterConsumerExecutor `. Is this a valid assumption? So, user
should package them accordingly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1723 (1.x) Introduce ClusterMetricsConsu...
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1352#discussion_r60570256
--- Diff:
storm-core/src/jvm/org/apache/storm/metric/ClusterConsumerExecutor.java ---
@@ -0,0 +1,87 @@
+/**
+ * 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.metric;
+
+import com.google.common.util.concurrent.MoreExecutors;
+import org.apache.storm.Config;
+import org.apache.storm.metric.api.DataPoint;
+import org.apache.storm.metric.api.IClusterMetricsConsumer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+public class ClusterConsumerExecutor {
+public static final Logger LOG =
LoggerFactory.getLogger(ClusterConsumerExecutor.class);
+
+private IClusterMetricsConsumer metricsConsumer;
+private String consumerClassName;
+private Object registerationArgument;
+private ExecutorService consumerExecutorService;
+
+public ClusterConsumerExecutor(String consumerClassName, Object
registerationArgument) {
+this.consumerClassName = consumerClassName;
+this.registerationArgument = registerationArgument;
--- End diff --
typo: `registerationArgument ` should be `registrationArgument `
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1723 (1.x) Introduce ClusterMetricsConsu...
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/1352#issuecomment-212880715 Github denies to show the changes of nimbus.clj. ;( Attached diff file. (Github also denies to upload file with 'diff' extension.) [STORM-1723-1.x-v1-diff.txt](https://github.com/apache/storm/files/229830/STORM-1723-1.x-v1-diff.txt) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-1723 (1.x) Introduce ClusterMetricsConsu...
GitHub user HeartSaVioR opened a pull request: https://github.com/apache/storm/pull/1352 STORM-1723 (1.x) Introduce ClusterMetricsConsumer * ClusterMetricsConsumer publishes cluster-side related metrics into consumers * like MetricsConsumer for topology metrics * Users can implement IClusterMetricsConsumer and configure to cluster conf. file to take effect * Please refer conf/storm.yaml.example for more details on configuring * Nimbus should be launched with additional jars which are needed for IClusterMetricsConsumer * Also did some refactor to nimbus.clj I don't address this for master yet since ui/core.clj is not converted to Java but stats.clj is converted to Java. If we want to move some methods in this patch to stats.clj, I'll follow up reviews, and start working on crafting patch against master when patch is ready to merge. You can merge this pull request into a Git repository by running: $ git pull https://github.com/HeartSaVioR/storm STORM-1723-1.x-branch Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/1352.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 #1352 commit 02a6764b766c85a19bb38dc971be8084c88a4508 Author: Jungtaek Lim Date: 2016-04-21T11:40:51Z STORM-1723 Introduce ClusterMetricsConsumer * ClusterMetricsConsumer publishes cluster-side related metrics into consumers * like MetricsConsumer for topology metrics * Users can implement IClusterMetricsConsumer and configure to cluster conf. file to take effect * Please refer conf/storm.yaml.example for more details on configuring * Nimbus should be launched with additional jars which are needed for IClusterMetricsConsumer * Also did some refactor to nimbus.clj --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
