----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48030/#review136411 -----------------------------------------------------------
Changes look good, only thing to consider is the changes to the metric name. Cluster Aggregation will not occur at topology level since appId = topologyName for metrics with the same metric name. Is the metric name to fine grained? Only thing to consider is whether we need task metrics to be aggregated across topology? If yes, taskId cannot be part of the metric name. Could you also add Aravindan Vijayan to the reviewers? Thanks. - Sid Wagle On May 30, 2016, 8:21 a.m., Jungtaek Lim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48030/ > ----------------------------------------------------------- > > (Updated May 30, 2016, 8:21 a.m.) > > > Review request for Ambari, Sriharsha Chintalapani and Sid Wagle. > > > Bugs: AMBARI-16946 > https://issues.apache.org/jira/browse/AMBARI-16946 > > > Repository: ambari > > > Description > ------- > > There's a mismatch between TimelineMetricsCache and Storm metrics unit, while > TimelineMetricsCache considers "metric name + timestamp" to be unique but > Storm is not. > > For example, assume that bolt B has task T1, T2 and B has registered metrics > M1. It's possible for metrics sink to receive (T1, M1) and (T2, M1) with same > timestamp TS1 (in TaskInfo, not current time), and received later will be > discarded from TimelineMetricsCache. > > If we want to have unique metric point of Storm, we should use "topology name > + component name + task id + metric name" to metric name so that "metric name > + timestamp" will be unique. > > There're other issues I would like to address, too. > > - Currently, hostname is written to hostname of the machine which runs > metrics sink. Since TaskInfo has hostname of the machine which runs task, > we're better to use this. > - Unit of timestamp of TaskInfo is second, while Storm Metrics Sink uses this > as millisecond, resulting in timestamp flaw, and malfunction of cache > eviction. It should be multiplied by 1000. > - 'component name' is not unique across the cluster, so it's not fit for app > id. 'topology name' is unique so proper value of app id is topology name. > > Consideration: Hostname for determining 'write shard' is set to hostname of > the machine which runs metrics sink. Since I don't know read also be sharded, > I'm not sure it's safe to use TaskInfo's hostname to hostname of > TimelineMetric. Please review carefully regarding this. > > > Diffs > ----- > > > ambari-metrics/ambari-metrics-storm-sink/src/main/java/org/apache/hadoop/metrics2/sink/storm/StormTimelineMetricsSink.java > 02f5598 > > ambari-metrics/ambari-metrics-storm-sink/src/test/java/org/apache/hadoop/metrics2/sink/storm/StormTimelineMetricsSinkTest.java > 8171a4d > > Diff: https://reviews.apache.org/r/48030/diff/ > > > Testing > ------- > > I tested this only ambari-metrics module since changeset is not related on > other modules. > > [INFO] > ------------------------------------------------------------------------ > [INFO] Reactor Summary: > [INFO] > [INFO] ambari-metrics ..................................... SUCCESS [ 0.896 > s] > [INFO] Ambari Metrics Common .............................. SUCCESS [ 13.386 > s] > [INFO] Ambari Metrics Hadoop Sink ......................... SUCCESS [ 5.085 > s] > [INFO] Ambari Metrics Flume Sink .......................... SUCCESS [ 6.827 > s] > [INFO] Ambari Metrics Kafka Sink .......................... SUCCESS [ 4.190 > s] > [INFO] Ambari Metrics Storm Sink .......................... SUCCESS [ 1.384 > s] > [INFO] Ambari Metrics Collector ........................... SUCCESS [04:06 > min] > [INFO] Ambari Metrics Monitor ............................. SUCCESS [ 3.556 > s] > [INFO] Ambari Metrics Grafana ............................. SUCCESS [01:03 > min] > [INFO] Ambari Metrics Assembly ............................ SUCCESS [ 3.567 > s] > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 05:48 min > [INFO] Finished at: 2016-05-30T16:46:07+09:00 > [INFO] Final Memory: 78M/1038M > [INFO] > ------------------------------------------------------------------------ > > In fact, I tried to run `mvn test` from ambari root directory but build is > failing from ambari-web. > > ``` > > fsevents@0.2.1 install > > /Users/jlim/WorkArea/JavaProjects/ambari/ambari-web/node_modules/chokidar/node_modules/fsevents > > node-gyp rebuild > ... > npm WARN install:fsevents fsevents@0.2.1 install: `node-gyp rebuild` > npm WARN install:fsevents Exit status 1 > ``` > > No luck on `npm install`, too. > > > Thanks, > > Jungtaek Lim > >
