----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44276/#review121709 -----------------------------------------------------------
ambari-metrics/ambari-metrics-timelineservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/HBaseTimelineMetricStore.java (line 100) <https://reviews.apache.org/r/44276/#comment183493> Why this change, rather pull up the method to the interface? ambari-metrics/ambari-metrics-timelineservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/HBaseTimelineMetricStore.java (line 343) <https://reviews.apache.org/r/44276/#comment183495> What if 2 aggregators of the same type run in parallel with the one already running has not save the checkpoint. We need a shared lock to gurantee this situation is handled correctly or we need to make sure that only 1 aggregator thread is running at any 1 time. ambari-metrics/ambari-metrics-timelineservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/aggregators/AbstractTimelineAggregator.java (line 317) <https://reviews.apache.org/r/44276/#comment183498> Rename to *Mills() to indicated everything in millis. ambari-metrics/ambari-metrics-timelineservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/aggregators/TimelineMetricAggregatorFactory.java (line 373) <https://reviews.apache.org/r/44276/#comment183501> Again return the interface vs concrete impl. ambari-metrics/ambari-metrics-timelineservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/aggregators/TimelineMetricClusterAggregator.java (line 107) <https://reviews.apache.org/r/44276/#comment183502> Seems ts is already present in constructor ambari-metrics/ambari-metrics-timelineservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/aggregators/v2/TimelineMetricClusterAggregator.java (line 75) <https://reviews.apache.org/r/44276/#comment183503> Shouldn't max of server time be alligned alredy, I think this might introduce test skew if there is any faulty login in aggregations. - Sid Wagle On March 2, 2016, 6:27 p.m., Aravindan Vijayan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44276/ > ----------------------------------------------------------- > > (Updated March 2, 2016, 6:27 p.m.) > > > Review request for Ambari, Dmytro Sen, Sumit Mohanty, and Sid Wagle. > > > Bugs: AMBARI-15267 > https://issues.apache.org/jira/browse/AMBARI-15267 > > > Repository: ambari > > > Description > ------- > > The timestamp of aggregated metrics is tied to service start time. For > example if the AMS service was started at 10:21, all hourly aggregated metric > will have timestamps like 10:21, 11:21, 12:21 and so on. > If AMS was restarted at 1:47, the subsequent hourly aggregates will have > timestamps like 1:47, 2:47, 3:47 and so on. > > This creates inconsistency and difficulty in using the metrics. All aggregate > timestamps should have definitive boundaries. For example, irrespective of > when the AMS was started, the hourly aggregate should always be timestamped > to top of hour (eg. all aggregated metrics having timestamp >= 10 AM and < > 11:00 AM should be timestamped to 11:00 AM ), and similarly 5 minute > aggregates should be timestamped to 0th, 5th, 10th, 15th..... minute > > > Diffs > ----- > > > ambari-metrics/ambari-metrics-timelineservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/HBaseTimelineMetricStore.java > 37e4796 > > ambari-metrics/ambari-metrics-timelineservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/aggregators/AbstractTimelineAggregator.java > fce5a39 > > ambari-metrics/ambari-metrics-timelineservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/aggregators/TimelineClusterMetric.java > 3c30a6f > > ambari-metrics/ambari-metrics-timelineservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/aggregators/TimelineMetricAggregatorFactory.java > cc85c56 > > ambari-metrics/ambari-metrics-timelineservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/aggregators/TimelineMetricClusterAggregator.java > 1c1c4b6 > > ambari-metrics/ambari-metrics-timelineservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/aggregators/TimelineMetricHostAggregator.java > e0fa26e > > ambari-metrics/ambari-metrics-timelineservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/aggregators/v2/TimelineMetricClusterAggregator.java > 5257412 > > ambari-metrics/ambari-metrics-timelineservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/aggregators/v2/TimelineMetricHostAggregator.java > 1c46642 > > ambari-metrics/ambari-metrics-timelineservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/query/PhoenixTransactSQL.java > 2fc6c34 > > ambari-metrics/ambari-metrics-timelineservice/src/test/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/ITPhoenixHBaseAccessor.java > e3e037a > > ambari-metrics/ambari-metrics-timelineservice/src/test/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/aggregators/AbstractTimelineAggregatorTest.java > 2b29469 > > ambari-metrics/ambari-metrics-timelineservice/src/test/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/aggregators/ITClusterAggregator.java > f201224 > > ambari-metrics/ambari-metrics-timelineservice/src/test/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/aggregators/ITMetricAggregator.java > 9c7c8fa > > Diff: https://reviews.apache.org/r/44276/diff/ > > > Testing > ------- > > Manually tested different scenarios. > > Added unit tests. > > ambari-metrics unit tests pass. > > > Thanks, > > Aravindan Vijayan > >
