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 2016/07/13 14:47:38 UTC
[GitHub] storm pull request #1560: STORM-1966 Expand metric having Map type as value ...
GitHub user HeartSaVioR opened a pull request:
https://github.com/apache/storm/pull/1560
STORM-1966 Expand metric having Map type as value into multiple metrics based on entries
* populate metric before applying filter so that filter can play with populated metrics
* add relevant configs to metrics consumer registration and describe them to storm.config.yaml
* add unit test
Please refer https://issues.apache.org/jira/browse/STORM-1966 for more details.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/HeartSaVioR/storm STORM-1966
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1560.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 #1560
----
commit 31cba7f69a3ad7220eb9036bdd28a356cbb9c7df
Author: Jungtaek Lim <ka...@gmail.com>
Date: 2016-07-13T08:23:26Z
STORM-1966 Expand metric having Map type as value into multiple metrics based on entries
* populate metric before applying filter so that filter can play with populated metrics
* add relevant configs to metrics consumer registration and describe them to storm.config.yaml
* add unit test
----
---
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 #1560: STORM-1966 Expand metric having Map type as value ...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1560#discussion_r70787015
--- Diff: storm-core/src/jvm/org/apache/storm/metric/util/DataPointPopulator.java ---
@@ -0,0 +1,61 @@
+package org.apache.storm.metric.util;
+
+import org.apache.storm.metric.api.IMetricsConsumer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+public class DataPointPopulator implements Serializable {
--- End diff --
I would prefer the name DataPointExpander. 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1560: STORM-1966 Expand metric having Map type as value ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1560#discussion_r70800325
--- Diff: storm-core/src/jvm/org/apache/storm/metric/util/DataPointPopulator.java ---
@@ -0,0 +1,61 @@
+package org.apache.storm.metric.util;
+
+import org.apache.storm.metric.api.IMetricsConsumer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+public class DataPointPopulator implements Serializable {
+ public static final Logger LOG = LoggerFactory.getLogger(DataPointPopulator.class);
+
+ private final boolean populateMapType;
+ private final String metricNameSeparator;
+
+ public DataPointPopulator(boolean populateMapType, String metricNameSeparator) {
+ this.populateMapType = populateMapType;
+ this.metricNameSeparator = metricNameSeparator;
+ }
+
+ public Collection<IMetricsConsumer.DataPoint> populateDataPoints(Collection<IMetricsConsumer.DataPoint> dataPoints) {
+ if (!populateMapType) {
+ return dataPoints;
+ }
+
+ List<IMetricsConsumer.DataPoint> populatedDataPoints = new ArrayList<>();
+
+ for (IMetricsConsumer.DataPoint dataPoint : dataPoints) {
+ populatedDataPoints.addAll(populateDataPoint(dataPoint));
+ }
+
+ return populatedDataPoints;
+ }
+
+ public Collection<IMetricsConsumer.DataPoint> populateDataPoint(IMetricsConsumer.DataPoint dataPoint) {
--- End diff --
At least built-in metrics have at most one depth Map, but no more (I mean no nested Map). The reason we would want to have such kind of logic is to handle built-in metrics. In normal, having Map as value is not recommended (which is what Bobby said too flexible).
For example, some metrics consumers take care of nested map, some of others only take care one depth map, the others don't take care of map. Users shouldn't rely on them.
---
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 #1560: STORM-1966 Expand metric having Map type as value ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1560#discussion_r70800422
--- Diff: storm-core/src/jvm/org/apache/storm/metric/util/DataPointPopulator.java ---
@@ -0,0 +1,61 @@
+package org.apache.storm.metric.util;
+
+import org.apache.storm.metric.api.IMetricsConsumer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+public class DataPointPopulator implements Serializable {
+ public static final Logger LOG = LoggerFactory.getLogger(DataPointPopulator.class);
+
+ private final boolean populateMapType;
+ private final String metricNameSeparator;
+
+ public DataPointPopulator(boolean populateMapType, String metricNameSeparator) {
+ this.populateMapType = populateMapType;
+ this.metricNameSeparator = metricNameSeparator;
+ }
+
+ public Collection<IMetricsConsumer.DataPoint> populateDataPoints(Collection<IMetricsConsumer.DataPoint> dataPoints) {
+ if (!populateMapType) {
+ return dataPoints;
+ }
+
+ List<IMetricsConsumer.DataPoint> populatedDataPoints = new ArrayList<>();
+
+ for (IMetricsConsumer.DataPoint dataPoint : dataPoints) {
+ populatedDataPoints.addAll(populateDataPoint(dataPoint));
+ }
+
+ return populatedDataPoints;
+ }
+
+ public Collection<IMetricsConsumer.DataPoint> populateDataPoint(IMetricsConsumer.DataPoint dataPoint) {
+ if (!populateMapType) {
+ return Collections.singletonList(dataPoint);
+ }
+
+ List<IMetricsConsumer.DataPoint> dataPoints = new ArrayList<>();
+
+ if (dataPoint.value == null) {
+ LOG.warn("Data point with name " + dataPoint.name + " is null. Discarding." + dataPoint.name);
+ } else if (dataPoint.value instanceof Map) {
+ Map<String, Object> dataMap = (Map<String, Object>) dataPoint.value;
--- End diff --
Yeah good point. Will address.
---
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 #1560: STORM-1966 Expand metric having Map type as value into mu...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1560
Thanks for thoughtful review, @abhishekagarwal87 ! Addressed your review comments.
---
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 #1560: STORM-1966 Expand metric having Map type as value ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1560#discussion_r70800633
--- Diff: storm-core/src/jvm/org/apache/storm/metric/util/DataPointPopulator.java ---
@@ -0,0 +1,61 @@
+package org.apache.storm.metric.util;
+
+import org.apache.storm.metric.api.IMetricsConsumer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+public class DataPointPopulator implements Serializable {
+ public static final Logger LOG = LoggerFactory.getLogger(DataPointPopulator.class);
+
+ private final boolean populateMapType;
+ private final String metricNameSeparator;
+
+ public DataPointPopulator(boolean populateMapType, String metricNameSeparator) {
+ this.populateMapType = populateMapType;
+ this.metricNameSeparator = metricNameSeparator;
+ }
+
+ public Collection<IMetricsConsumer.DataPoint> populateDataPoints(Collection<IMetricsConsumer.DataPoint> dataPoints) {
+ if (!populateMapType) {
+ return dataPoints;
+ }
+
+ List<IMetricsConsumer.DataPoint> populatedDataPoints = new ArrayList<>();
+
+ for (IMetricsConsumer.DataPoint dataPoint : dataPoints) {
+ populatedDataPoints.addAll(populateDataPoint(dataPoint));
+ }
+
+ return populatedDataPoints;
+ }
+
+ public Collection<IMetricsConsumer.DataPoint> populateDataPoint(IMetricsConsumer.DataPoint dataPoint) {
+ if (!populateMapType) {
+ return Collections.singletonList(dataPoint);
+ }
+
+ List<IMetricsConsumer.DataPoint> dataPoints = new ArrayList<>();
+
+ if (dataPoint.value == null) {
+ LOG.warn("Data point with name " + dataPoint.name + " is null. Discarding." + dataPoint.name);
--- End diff --
Thanks for finding. I copied the code from Ambari metrics Storm sink which uses Log4j so missed this. 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1560: STORM-1966 Expand metric having Map type as value ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1560#discussion_r70801243
--- Diff: storm-core/src/jvm/org/apache/storm/metric/api/IMetricsConsumer.java ---
@@ -54,6 +54,25 @@ public String toString() {
}
public String name;
public Object value;
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) return true;
+ if (!(o instanceof DataPoint)) return false;
+
+ DataPoint dataPoint = (DataPoint) o;
+
+ if (name != null ? !name.equals(dataPoint.name) : dataPoint.name != null) return false;
--- End diff --
This is IntelliJ generated code so might not beauty and doesn't rely on other library. StringUtils.equals() would be better. 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1560: STORM-1966 Expand metric having Map type as value ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1560#discussion_r70800983
--- Diff: conf/storm.yaml.example ---
@@ -45,6 +45,10 @@
## - when none of configuration for metric filter are specified, it'll be treated as 'pass all'.
## - you need to specify either whitelist or blacklist, or none of them. You can't specify both of them.
## - you can specify multiple whitelist / blacklist with regular expression
+## populateMapType: expand metric with map type as value to multiple metrics
+## - set to true when you would like to apply filter to populated metrics
--- End diff --
Yes I was thinking about that but missed it. 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1560: STORM-1966 Expand metric having Map type as value ...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1560#discussion_r70786295
--- Diff: storm-core/src/jvm/org/apache/storm/metric/api/IMetricsConsumer.java ---
@@ -54,6 +54,25 @@ public String toString() {
}
public String name;
public Object value;
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) return true;
+ if (!(o instanceof DataPoint)) return false;
+
+ DataPoint dataPoint = (DataPoint) o;
+
+ if (name != null ? !name.equals(dataPoint.name) : dataPoint.name != null) return false;
--- End diff --
it will be good to have parantheses around individual comparisons e.g. (name != null) or it can be simplified to StringUtils.equals(name, dataPoint.name)
---
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 #1560: STORM-1966 Expand metric having Map type as value ...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1560#discussion_r70787070
--- Diff: storm-core/src/jvm/org/apache/storm/metric/util/DataPointPopulator.java ---
@@ -0,0 +1,61 @@
+package org.apache.storm.metric.util;
+
+import org.apache.storm.metric.api.IMetricsConsumer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+public class DataPointPopulator implements Serializable {
+ public static final Logger LOG = LoggerFactory.getLogger(DataPointPopulator.class);
+
+ private final boolean populateMapType;
+ private final String metricNameSeparator;
+
+ public DataPointPopulator(boolean populateMapType, String metricNameSeparator) {
+ this.populateMapType = populateMapType;
+ this.metricNameSeparator = metricNameSeparator;
+ }
+
+ public Collection<IMetricsConsumer.DataPoint> populateDataPoints(Collection<IMetricsConsumer.DataPoint> dataPoints) {
+ if (!populateMapType) {
+ return dataPoints;
+ }
+
+ List<IMetricsConsumer.DataPoint> populatedDataPoints = new ArrayList<>();
+
+ for (IMetricsConsumer.DataPoint dataPoint : dataPoints) {
+ populatedDataPoints.addAll(populateDataPoint(dataPoint));
+ }
+
+ return populatedDataPoints;
+ }
+
+ public Collection<IMetricsConsumer.DataPoint> populateDataPoint(IMetricsConsumer.DataPoint dataPoint) {
+ if (!populateMapType) {
+ return Collections.singletonList(dataPoint);
+ }
+
+ List<IMetricsConsumer.DataPoint> dataPoints = new ArrayList<>();
+
+ if (dataPoint.value == null) {
+ LOG.warn("Data point with name " + dataPoint.name + " is null. Discarding." + dataPoint.name);
--- End diff --
let's use {} syntax for consistency
---
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 #1560: STORM-1966 Expand metric having Map type as value into mu...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/1560
+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 issue #1560: STORM-1966 Expand metric having Map type as value into mu...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1560
@harshach Could you take a look?
---
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 #1560: STORM-1966 Expand metric having Map type as value into mu...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/1560
@HeartSaVioR looks good to me. Just a minor nitpick +1 after that.
---
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 #1560: STORM-1966 Expand metric having Map type as value into mu...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1560
I also addressed review comments and also RAT issue to 1.x PR.
Please take a look at #1561 as well. Thanks!
---
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 #1560: STORM-1966 Expand metric having Map type as value ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1560#discussion_r70804467
--- Diff: storm-core/src/jvm/org/apache/storm/metric/api/IMetricsConsumer.java ---
@@ -54,6 +54,25 @@ public String toString() {
}
public String name;
public Object value;
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) return true;
+ if (!(o instanceof DataPoint)) return false;
+
+ DataPoint dataPoint = (DataPoint) o;
+
+ if (name != null ? !name.equals(dataPoint.name) : dataPoint.name != null) return false;
--- End diff --
Since we're on JDK7, I can just use Objects.equals and Objects.deepEquals.
---
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 #1560: STORM-1966 Expand metric having Map type as value into mu...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/1560
Thanks @HeartSaVioR . +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 #1560: STORM-1966 Expand metric having Map type as value ...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1560#discussion_r70786519
--- Diff: conf/storm.yaml.example ---
@@ -45,6 +45,10 @@
## - when none of configuration for metric filter are specified, it'll be treated as 'pass all'.
## - you need to specify either whitelist or blacklist, or none of them. You can't specify both of them.
## - you can specify multiple whitelist / blacklist with regular expression
+## populateMapType: expand metric with map type as value to multiple metrics
+## - set to true when you would like to apply filter to populated metrics
--- End diff --
Please document the default value as well
---
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 #1560: STORM-1966 Expand metric having Map type as value ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1560#discussion_r70809388
--- Diff: storm-core/src/jvm/org/apache/storm/metric/MetricsConsumerBolt.java ---
@@ -43,19 +44,21 @@
OutputCollector _collector;
Object _registrationArgument;
private final int _maxRetainMetricTuples;
- private Predicate<IMetricsConsumer.DataPoint> _filterPredicate;
+ private final Predicate<IMetricsConsumer.DataPoint> _filterPredicate;
+ private final DataPointExpander _populator;
private final BlockingQueue<MetricsTask> _taskQueue;
private Thread _taskExecuteThread;
private volatile boolean _running = true;
public MetricsConsumerBolt(String consumerClassName, Object registrationArgument, int maxRetainMetricTuples,
- Predicate<IMetricsConsumer.DataPoint> filterPredicate) {
+ Predicate<IMetricsConsumer.DataPoint> filterPredicate, DataPointExpander populator) {
--- End diff --
Oh I missed that. Thanks!
---
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 #1560: STORM-1966 Expand metric having Map type as value into mu...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1560
Addressed and squashed.
---
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 #1560: STORM-1966 Expand metric having Map type as value ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1560#discussion_r70801003
--- Diff: conf/storm.yaml.example ---
@@ -45,6 +45,10 @@
## - when none of configuration for metric filter are specified, it'll be treated as 'pass all'.
## - you need to specify either whitelist or blacklist, or none of them. You can't specify both of them.
## - you can specify multiple whitelist / blacklist with regular expression
+## populateMapType: expand metric with map type as value to multiple metrics
--- End diff --
OK Will do.
---
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 #1560: STORM-1966 Expand metric having Map type as value into mu...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the issue:
https://github.com/apache/storm/pull/1560
Minor nit regarding variable name. Other than that +1.
Please make sure to squash commits before you merge :)
---
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 #1560: STORM-1966 Expand metric having Map type as value ...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1560#discussion_r70787174
--- Diff: storm-core/src/jvm/org/apache/storm/metric/util/DataPointPopulator.java ---
@@ -0,0 +1,61 @@
+package org.apache.storm.metric.util;
+
+import org.apache.storm.metric.api.IMetricsConsumer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+public class DataPointPopulator implements Serializable {
+ public static final Logger LOG = LoggerFactory.getLogger(DataPointPopulator.class);
+
+ private final boolean populateMapType;
+ private final String metricNameSeparator;
+
+ public DataPointPopulator(boolean populateMapType, String metricNameSeparator) {
+ this.populateMapType = populateMapType;
+ this.metricNameSeparator = metricNameSeparator;
+ }
+
+ public Collection<IMetricsConsumer.DataPoint> populateDataPoints(Collection<IMetricsConsumer.DataPoint> dataPoints) {
+ if (!populateMapType) {
+ return dataPoints;
+ }
+
+ List<IMetricsConsumer.DataPoint> populatedDataPoints = new ArrayList<>();
+
+ for (IMetricsConsumer.DataPoint dataPoint : dataPoints) {
+ populatedDataPoints.addAll(populateDataPoint(dataPoint));
+ }
+
+ return populatedDataPoints;
+ }
+
+ public Collection<IMetricsConsumer.DataPoint> populateDataPoint(IMetricsConsumer.DataPoint dataPoint) {
+ if (!populateMapType) {
+ return Collections.singletonList(dataPoint);
+ }
+
+ List<IMetricsConsumer.DataPoint> dataPoints = new ArrayList<>();
+
+ if (dataPoint.value == null) {
+ LOG.warn("Data point with name " + dataPoint.name + " is null. Discarding." + dataPoint.name);
+ } else if (dataPoint.value instanceof Map) {
+ Map<String, Object> dataMap = (Map<String, Object>) dataPoint.value;
--- End diff --
There is no guarantee that key type will be string. Might be better to assume it to of Object type and wrap entry.getKey() in String.valueOf
---
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 #1560: STORM-1966 Expand metric having Map type as value into mu...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the issue:
https://github.com/apache/storm/pull/1560
Thanks @HeartSaVioR . Looks good to me
---
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 #1560: STORM-1966 Expand metric having Map type as value ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1560
---
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 #1560: STORM-1966 Expand metric having Map type as value ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1560#discussion_r70800924
--- Diff: storm-core/src/jvm/org/apache/storm/metric/util/DataPointPopulator.java ---
@@ -0,0 +1,61 @@
+package org.apache.storm.metric.util;
+
+import org.apache.storm.metric.api.IMetricsConsumer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+public class DataPointPopulator implements Serializable {
--- End diff --
Yes `expand` would be better. I was not sure which word is good to represent the behavior. We can replace all `populate` to `expand`.
---
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 #1560: STORM-1966 Expand metric having Map type as value ...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1560#discussion_r70786482
--- Diff: conf/storm.yaml.example ---
@@ -45,6 +45,10 @@
## - when none of configuration for metric filter are specified, it'll be treated as 'pass all'.
## - you need to specify either whitelist or blacklist, or none of them. You can't specify both of them.
## - you can specify multiple whitelist / blacklist with regular expression
+## populateMapType: expand metric with map type as value to multiple metrics
--- End diff --
can this be renamed to expandMapType?
---
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 #1560: STORM-1966 Expand metric having Map type as value ...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on a diff in the pull request:
https://github.com/apache/storm/pull/1560#discussion_r70856636
--- Diff: storm-core/src/jvm/org/apache/storm/daemon/StormCommon.java ---
@@ -391,9 +388,13 @@ public static void addEventLogger(Map conf, StormTopology topology) {
List<String> whitelist = (List<String>) info.get("whitelist");
List<String> blacklist = (List<String>) info.get("blacklist");
FilterByMetricName filterPredicate = new FilterByMetricName(whitelist, blacklist);
+ Boolean expandMapType = Utils.getBoolean(info.get("expandMapType"), false);
--- End diff --
can we make these strings into some final String constants
---
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 #1560: STORM-1966 Expand metric having Map type as value ...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1560#discussion_r70787294
--- Diff: storm-core/src/jvm/org/apache/storm/metric/util/DataPointPopulator.java ---
@@ -0,0 +1,61 @@
+package org.apache.storm.metric.util;
+
+import org.apache.storm.metric.api.IMetricsConsumer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+public class DataPointPopulator implements Serializable {
+ public static final Logger LOG = LoggerFactory.getLogger(DataPointPopulator.class);
+
+ private final boolean populateMapType;
+ private final String metricNameSeparator;
+
+ public DataPointPopulator(boolean populateMapType, String metricNameSeparator) {
+ this.populateMapType = populateMapType;
+ this.metricNameSeparator = metricNameSeparator;
+ }
+
+ public Collection<IMetricsConsumer.DataPoint> populateDataPoints(Collection<IMetricsConsumer.DataPoint> dataPoints) {
+ if (!populateMapType) {
+ return dataPoints;
+ }
+
+ List<IMetricsConsumer.DataPoint> populatedDataPoints = new ArrayList<>();
+
+ for (IMetricsConsumer.DataPoint dataPoint : dataPoints) {
+ populatedDataPoints.addAll(populateDataPoint(dataPoint));
+ }
+
+ return populatedDataPoints;
+ }
+
+ public Collection<IMetricsConsumer.DataPoint> populateDataPoint(IMetricsConsumer.DataPoint dataPoint) {
--- End diff --
this currently does not handle nested maps. Is that intended?
---
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 #1560: STORM-1966 Expand metric having Map type as value ...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1560#discussion_r70808499
--- Diff: storm-core/src/jvm/org/apache/storm/metric/MetricsConsumerBolt.java ---
@@ -43,19 +44,21 @@
OutputCollector _collector;
Object _registrationArgument;
private final int _maxRetainMetricTuples;
- private Predicate<IMetricsConsumer.DataPoint> _filterPredicate;
+ private final Predicate<IMetricsConsumer.DataPoint> _filterPredicate;
+ private final DataPointExpander _populator;
private final BlockingQueue<MetricsTask> _taskQueue;
private Thread _taskExecuteThread;
private volatile boolean _running = true;
public MetricsConsumerBolt(String consumerClassName, Object registrationArgument, int maxRetainMetricTuples,
- Predicate<IMetricsConsumer.DataPoint> filterPredicate) {
+ Predicate<IMetricsConsumer.DataPoint> filterPredicate, DataPointExpander populator) {
--- End diff --
Minor nit. populator variable name can be changed as well.
---
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 #1560: STORM-1966 Expand metric having Map type as value into mu...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1560
@harshach Addressed.
I found RAT issue again (my bad) and fixed it.
Btw, I didn't address review comments to PR for 1.x-branch. Once I'm done I'll mention reviewers to review from PR for 1.x-branch as well.
---
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.
---