You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by HeartSaVioR <gi...@git.apache.org> on 2017/01/17 14:53:46 UTC
[GitHub] storm pull request #1881: STORM-2298 Don't kill Nimbus when ClusterMetricsCo...
GitHub user HeartSaVioR opened a pull request:
https://github.com/apache/storm/pull/1881
STORM-2298 Don't kill Nimbus when ClusterMetricsConsumer is failed to initialize
Please refer [STORM-2298](https://issues.apache.org/jira/browse/STORM-2298) for more details.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/HeartSaVioR/storm STORM-2298
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1881.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 #1881
----
commit 1e04a3f7041874ee709d2812d6a823ab17d5a2e2
Author: Jungtaek Lim <ka...@gmail.com>
Date: 2017-01-17T14:51:27Z
STORM-2298 Don't kill Nimbus when ClusterMetricsConsumer is failed to initialize
* don't propagate errors, but just retry when needed
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1881: STORM-2298 Don't kill Nimbus when ClusterMetricsCo...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on a diff in the pull request:
https://github.com/apache/storm/pull/1881#discussion_r96769031
--- Diff: storm-core/src/jvm/org/apache/storm/metric/ClusterMetricsConsumerExecutor.java ---
@@ -40,31 +39,49 @@ public ClusterMetricsConsumerExecutor(String consumerClassName, Object registrat
public void prepare() {
try {
metricsConsumer = (IClusterMetricsConsumer)Class.forName(consumerClassName).newInstance();
+ metricsConsumer.prepare(registrationArgument);
} catch (Exception e) {
- throw new RuntimeException("Could not instantiate a class listed in config under section " +
- Config.STORM_CLUSTER_METRICS_CONSUMER_REGISTER + " with fully qualified name " + consumerClassName, e);
- }
+ LOG.error("Could not instantiate or prepare Cluster Metrics Consumer with fully qualified name " +
+ consumerClassName, e);
- metricsConsumer.prepare(registrationArgument);
+ if (metricsConsumer != null) {
+ metricsConsumer.cleanup();
+ }
+ metricsConsumer = null;
+ }
}
public void handleDataPoints(final IClusterMetricsConsumer.ClusterInfo clusterInfo, final Collection<DataPoint> dataPoints) {
- try {
- metricsConsumer.handleDataPoints(clusterInfo, dataPoints);
- } catch (Throwable e) {
- LOG.error("Error while handling cluster data points, consumer class: " + consumerClassName, e);
+ if (metricsConsumer == null) {
--- End diff --
@HeartSaVioR I am worried about the state where its badly configured and its retrying every scheduled interval. I think we can move it to try initializing only once and log an error if its fails to do so.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1881: STORM-2298 Don't kill Nimbus when ClusterMetricsConsumer ...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/1881
+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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1881: STORM-2298 Don't kill Nimbus when ClusterMetricsCo...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1881#discussion_r96588990
--- Diff: storm-core/src/jvm/org/apache/storm/metric/ClusterMetricsConsumerExecutor.java ---
@@ -40,31 +39,49 @@ public ClusterMetricsConsumerExecutor(String consumerClassName, Object registrat
public void prepare() {
try {
metricsConsumer = (IClusterMetricsConsumer)Class.forName(consumerClassName).newInstance();
+ metricsConsumer.prepare(registrationArgument);
} catch (Exception e) {
- throw new RuntimeException("Could not instantiate a class listed in config under section " +
- Config.STORM_CLUSTER_METRICS_CONSUMER_REGISTER + " with fully qualified name " + consumerClassName, e);
- }
+ LOG.error("Could not instantiate or prepare Cluster Metrics Consumer with fully qualified name " +
+ consumerClassName, e);
- metricsConsumer.prepare(registrationArgument);
+ if (metricsConsumer != null) {
+ metricsConsumer.cleanup();
+ }
+ metricsConsumer = null;
+ }
}
public void handleDataPoints(final IClusterMetricsConsumer.ClusterInfo clusterInfo, final Collection<DataPoint> dataPoints) {
- try {
- metricsConsumer.handleDataPoints(clusterInfo, dataPoints);
- } catch (Throwable e) {
- LOG.error("Error while handling cluster data points, consumer class: " + consumerClassName, e);
+ if (metricsConsumer == null) {
--- End diff --
Some modules (at least what I've reported) could throw error on initialization phase when backend storage is not working properly. This could be intermittent so I thought it might be better to retry when needed, but I'm also OK to remove retry logic and leave error log if we don't want to retry automatically.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1881: STORM-2298 Don't kill Nimbus when ClusterMetricsConsumer ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1881
@harshach Thanks for reviewing. I addressed your comment.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1881: STORM-2298 Don't kill Nimbus when ClusterMetricsCo...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1881#discussion_r96560105
--- Diff: storm-core/src/jvm/org/apache/storm/metric/ClusterMetricsConsumerExecutor.java ---
@@ -40,31 +39,49 @@ public ClusterMetricsConsumerExecutor(String consumerClassName, Object registrat
public void prepare() {
try {
metricsConsumer = (IClusterMetricsConsumer)Class.forName(consumerClassName).newInstance();
+ metricsConsumer.prepare(registrationArgument);
} catch (Exception e) {
- throw new RuntimeException("Could not instantiate a class listed in config under section " +
- Config.STORM_CLUSTER_METRICS_CONSUMER_REGISTER + " with fully qualified name " + consumerClassName, e);
- }
+ LOG.error("Could not instantiate or prepare Cluster Metrics Consumer with fully qualified name " +
+ consumerClassName, e);
- metricsConsumer.prepare(registrationArgument);
+ if (metricsConsumer != null) {
--- End diff --
This is to clean up broken metrics consumer instance, so only needed for error case.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1881: STORM-2298 Don't kill Nimbus when ClusterMetricsCo...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on a diff in the pull request:
https://github.com/apache/storm/pull/1881#discussion_r96560196
--- Diff: storm-core/src/jvm/org/apache/storm/metric/ClusterMetricsConsumerExecutor.java ---
@@ -40,31 +39,49 @@ public ClusterMetricsConsumerExecutor(String consumerClassName, Object registrat
public void prepare() {
try {
metricsConsumer = (IClusterMetricsConsumer)Class.forName(consumerClassName).newInstance();
+ metricsConsumer.prepare(registrationArgument);
} catch (Exception e) {
- throw new RuntimeException("Could not instantiate a class listed in config under section " +
- Config.STORM_CLUSTER_METRICS_CONSUMER_REGISTER + " with fully qualified name " + consumerClassName, e);
- }
+ LOG.error("Could not instantiate or prepare Cluster Metrics Consumer with fully qualified name " +
+ consumerClassName, e);
- metricsConsumer.prepare(registrationArgument);
+ if (metricsConsumer != null) {
+ metricsConsumer.cleanup();
+ }
+ metricsConsumer = null;
+ }
}
public void handleDataPoints(final IClusterMetricsConsumer.ClusterInfo clusterInfo, final Collection<DataPoint> dataPoints) {
- try {
- metricsConsumer.handleDataPoints(clusterInfo, dataPoints);
- } catch (Throwable e) {
- LOG.error("Error while handling cluster data points, consumer class: " + consumerClassName, e);
+ if (metricsConsumer == null) {
--- End diff --
do we need to attempt to re-initialize the metricsConsumer in handleDataPoints. Given that this method gets executed in a schedule-recurring loop.
If the metrics reporter isn't initialized it means that either the user is mis-configured or didn't supply the required class. Any of these errors means user need to fix and restart the nimbus.
So in that case it would be save to initialize the metrics reporter only once on nimbus start and we can probably add a debug message in the handleDataPoints if metricsConsumer is null so that users can figure out if the metrics are not getting published.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1881: STORM-2298 Don't kill Nimbus when ClusterMetricsCo...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1881#discussion_r96770202
--- Diff: storm-core/src/jvm/org/apache/storm/metric/ClusterMetricsConsumerExecutor.java ---
@@ -40,31 +39,49 @@ public ClusterMetricsConsumerExecutor(String consumerClassName, Object registrat
public void prepare() {
try {
metricsConsumer = (IClusterMetricsConsumer)Class.forName(consumerClassName).newInstance();
+ metricsConsumer.prepare(registrationArgument);
} catch (Exception e) {
- throw new RuntimeException("Could not instantiate a class listed in config under section " +
- Config.STORM_CLUSTER_METRICS_CONSUMER_REGISTER + " with fully qualified name " + consumerClassName, e);
- }
+ LOG.error("Could not instantiate or prepare Cluster Metrics Consumer with fully qualified name " +
+ consumerClassName, e);
- metricsConsumer.prepare(registrationArgument);
+ if (metricsConsumer != null) {
+ metricsConsumer.cleanup();
+ }
+ metricsConsumer = null;
+ }
}
public void handleDataPoints(final IClusterMetricsConsumer.ClusterInfo clusterInfo, final Collection<DataPoint> dataPoints) {
- try {
- metricsConsumer.handleDataPoints(clusterInfo, dataPoints);
- } catch (Throwable e) {
- LOG.error("Error while handling cluster data points, consumer class: " + consumerClassName, e);
+ if (metricsConsumer == null) {
--- End diff --
@harshach OK. I'll just make it leaving error log repeatedly on handleDataPoints and don't try initializing again.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1881: STORM-2298 Don't kill Nimbus when ClusterMetricsCo...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on a diff in the pull request:
https://github.com/apache/storm/pull/1881#discussion_r96559764
--- Diff: storm-core/src/jvm/org/apache/storm/metric/ClusterMetricsConsumerExecutor.java ---
@@ -40,31 +39,49 @@ public ClusterMetricsConsumerExecutor(String consumerClassName, Object registrat
public void prepare() {
try {
metricsConsumer = (IClusterMetricsConsumer)Class.forName(consumerClassName).newInstance();
+ metricsConsumer.prepare(registrationArgument);
} catch (Exception e) {
- throw new RuntimeException("Could not instantiate a class listed in config under section " +
- Config.STORM_CLUSTER_METRICS_CONSUMER_REGISTER + " with fully qualified name " + consumerClassName, e);
- }
+ LOG.error("Could not instantiate or prepare Cluster Metrics Consumer with fully qualified name " +
+ consumerClassName, e);
- metricsConsumer.prepare(registrationArgument);
+ if (metricsConsumer != null) {
--- End diff --
do we want to move this to finally clause
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1881: STORM-2298 Don't kill Nimbus when ClusterMetricsCo...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1881#discussion_r96775630
--- Diff: storm-core/src/jvm/org/apache/storm/metric/ClusterMetricsConsumerExecutor.java ---
@@ -40,31 +39,49 @@ public ClusterMetricsConsumerExecutor(String consumerClassName, Object registrat
public void prepare() {
try {
metricsConsumer = (IClusterMetricsConsumer)Class.forName(consumerClassName).newInstance();
+ metricsConsumer.prepare(registrationArgument);
} catch (Exception e) {
- throw new RuntimeException("Could not instantiate a class listed in config under section " +
- Config.STORM_CLUSTER_METRICS_CONSUMER_REGISTER + " with fully qualified name " + consumerClassName, e);
- }
+ LOG.error("Could not instantiate or prepare Cluster Metrics Consumer with fully qualified name " +
+ consumerClassName, e);
- metricsConsumer.prepare(registrationArgument);
+ if (metricsConsumer != null) {
+ metricsConsumer.cleanup();
+ }
+ metricsConsumer = null;
+ }
}
public void handleDataPoints(final IClusterMetricsConsumer.ClusterInfo clusterInfo, final Collection<DataPoint> dataPoints) {
- try {
- metricsConsumer.handleDataPoints(clusterInfo, dataPoints);
- } catch (Throwable e) {
- LOG.error("Error while handling cluster data points, consumer class: " + consumerClassName, e);
+ if (metricsConsumer == null) {
--- End diff --
@harshach 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1881: STORM-2298 Don't kill Nimbus when ClusterMetricsCo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1881
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---